Bug 226994

Summary: Add basic detection of unresponsive Network / GPU Processes
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: WebKit2Assignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: ggaren, jer.noble, kkinnunen, sihui_liu, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=230016
Attachments:
Description Flags
Patch
none
Patch none

Description Chris Dumez 2021-06-14 15:52:10 PDT
Add basic detection of unresponsive Network / GPU Processes.
Comment 1 Chris Dumez 2021-06-14 16:00:17 PDT
Created attachment 431376 [details]
Patch
Comment 2 Chris Dumez 2021-06-15 09:04:46 PDT
Created attachment 431445 [details]
Patch
Comment 3 Geoffrey Garen 2021-06-15 11:03:59 PDT
Comment on attachment 431445 [details]
Patch

r=me
Comment 4 Geoffrey Garen 2021-06-15 11:07:34 PDT
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).
Comment 5 Chris Dumez 2021-06-15 11:30:58 PDT
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()?
Comment 6 EWS 2021-06-15 13:53:35 PDT
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].
Comment 7 Radar WebKit Bug Importer 2021-06-15 13:54:16 PDT
<rdar://problem/79360129>
Comment 8 Geoffrey Garen 2021-06-16 10:29:55 PDT
> >> 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.)
Comment 9 Chris Dumez 2021-06-16 10:32:59 PDT
(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().
Comment 10 Sihui Liu 2021-09-08 14:33:38 PDT
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.