Bug 226994

Created attachment 431376 [details] Patch

Created attachment 431445 [details] Patch

Comment on attachment 431445 [details] Patch r=me

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 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()?

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].

<rdar://problem/79360129>

> >> 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.)

(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().

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.