Bug 226994 – Add basic detection of unresponsive Network / GPU Processes

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Chris Dumez]

Add basic detection of unresponsive Network / GPU Processes.

[Chris Dumez]

Created attachment 431376 [details]
Patch

[Chris Dumez]

Created attachment 431445 [details]
Patch

[Geoffrey Garen]

Comment on attachment 431445 [details]
Patch

r=me

[Geoffrey Garen]

Comment on attachment 431445 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=431445&action=review

> Source/WebKit/UIProcess/AuxiliaryProcessProxy.cpp:355
> +void AuxiliaryProcessProxy::startResponsivenessTimer(UseLazyStop useLazyStop)

Not new to this patch, but I think the code would be clearer if it were two separate functions: startResponsivenessTimerAfterLaunch() (conditional) and startResponsivenessTimer() (unconditional).

[Chris Dumez]

Comment on attachment 431445 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=431445&action=review

>> Source/WebKit/UIProcess/AuxiliaryProcessProxy.cpp:355
>> +void AuxiliaryProcessProxy::startResponsivenessTimer(UseLazyStop useLazyStop)
> 
> Not new to this patch, but I think the code would be clearer if it were two separate functions: startResponsivenessTimerAfterLaunch() (conditional) and startResponsivenessTimer() (unconditional).

But then nobody would call startResponsivenessTimerAfterLaunch() besides startResponsivenessTimer()?

[EWS]

Committed r278895 (238837@main): <https://commits.webkit.org/238837@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 431445 [details].

[Radar WebKit Bug Importer]

<rdar://problem/79360129>

[Geoffrey Garen]

> >> Source/WebKit/UIProcess/AuxiliaryProcessProxy.cpp:355
> >> +void AuxiliaryProcessProxy::startResponsivenessTimer(UseLazyStop useLazyStop)
> > 
> > Not new to this patch, but I think the code would be clearer if it were two separate functions: startResponsivenessTimerAfterLaunch() (conditional) and startResponsivenessTimer() (unconditional).
> 
> But then nobody would call startResponsivenessTimerAfterLaunch() besides
> startResponsivenessTimer()?

I would have all callers call startResponsivenessTimerAfterLaunch(), except for startResponsivenessTimer() and didFinishLaunching().

(Would also be OK if didFinishLaunching() called startResponsivenessTimerAfterLaunch(), I guess, just a little unnecessary; but the rename clarity would still hold, I think.)

[Chris Dumez]

(In reply to Geoffrey Garen from comment #8)
> > >> Source/WebKit/UIProcess/AuxiliaryProcessProxy.cpp:355
> > >> +void AuxiliaryProcessProxy::startResponsivenessTimer(UseLazyStop useLazyStop)
> > > 
> > > Not new to this patch, but I think the code would be clearer if it were two separate functions: startResponsivenessTimerAfterLaunch() (conditional) and startResponsivenessTimer() (unconditional).
> > 
> > But then nobody would call startResponsivenessTimerAfterLaunch() besides
> > startResponsivenessTimer()?
> 
> I would have all callers call startResponsivenessTimerAfterLaunch(), except
> for startResponsivenessTimer() and didFinishLaunching().
> 
> (Would also be OK if didFinishLaunching() called
> startResponsivenessTimerAfterLaunch(), I guess, just a little unnecessary;
> but the rename clarity would still hold, I think.)

Callers don't know if the process is done launching. They would have to check if the process is already launched before calling startResponsivenessTimerAfterLaunch().

[Sihui Liu]

We decided to hold off using responsiveness timer in NetworkProcessProxy in https://bugs.webkit.org/show_bug.cgi?id=230016, because network process gets killed too often for hangs (or some slow operations) and it leads to web process crashes. Relaunching network process seems to not resolve the issue.

We may turn it on after we identify the root cause and get it fixed.