Bug 230016 – Remove responsiveness timer in NetworkProcessProxy::getNetworkProcessConnection

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sihui Liu]

...

[Sihui Liu]

Created attachment 437543 [details]
Patch

[Chris Dumez]

Comment on attachment 437543 [details]
Patch

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

> Source/WebKit/ChangeLog:9
> +        responsiveness timer in NetworkProcessProxy::getNetworkProcessConnection, blocked netowrk process will be killed

typo: netowrk

> Source/WebKit/ChangeLog:10
> +        and a new network process will be launched for getting connection. However, the new network process may stuck

"may get stuck"

> Source/WebKit/ChangeLog:11
> +        in initializeNetworkProcess too. In this case, web process will crash for not able to get network process 

"for not being able"

> Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:-253
> -    startResponsivenessTimer(UseLazyStop::No);

What if we have a hung network process? Now we won't recover.

This patch is a revert of https://bugs.webkit.org/show_bug.cgi?id=226994 without trying to resolve the problem https://bugs.webkit.org/show_bug.cgi?id=226994 was addressing.

[Chris Dumez]

Comment on attachment 437543 [details]
Patch

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

>> Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:-253
>> -    startResponsivenessTimer(UseLazyStop::No);
> 
> What if we have a hung network process? Now we won't recover.
> 
> This patch is a revert of https://bugs.webkit.org/show_bug.cgi?id=226994 without trying to resolve the problem https://bugs.webkit.org/show_bug.cgi?id=226994 was addressing.

Maybe we only want to start the responsiveness timer if state() == Running to avoid starting it in the "process launch" case. That said, if the process hangs on launch, I believe we want and try to recover too so it would not be great.

Alternatively, we can use a longer responsiveness timer if we believe the process may take more than 3 seconds to launch but still eventually complete launching. It's hard to tell without data but dropping hang detection completely seems wrong.

[Sihui Liu]

(In reply to Chris Dumez from comment #3)
> Comment on attachment 437543 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=437543&action=review
> 
> >> Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:-253
> >> -    startResponsivenessTimer(UseLazyStop::No);
> > 
> > What if we have a hung network process? Now we won't recover.
> > 
> > This patch is a revert of https://bugs.webkit.org/show_bug.cgi?id=226994 without trying to resolve the problem https://bugs.webkit.org/show_bug.cgi?id=226994 was addressing.
> 
> Maybe we only want to start the responsiveness timer if state() == Running
> to avoid starting it in the "process launch" case. That said, if the process
> hangs on launch, I believe we want and try to recover too so it would not be
> great.

Hmm but state() == Running does not mean process does not hang. In this case network process is processing initializeNetowrkProcess message.
One problem for killing network process when it hangs is we won't know why it hangs. 

> 
> Alternatively, we can use a longer responsiveness timer if we believe the
> process may take more than 3 seconds to launch but still eventually complete
> launching. It's hard to tell without data but dropping hang detection
> completely seems wrong.

According to rdar://78729329(or rdar://78636467), the hang can be very long, which is a bug but may not get fixed soon.

[Sihui Liu]

Created attachment 437553 [details]
Patch

[Sihui Liu]

rdar://80760179

[Sihui Liu]

Created attachment 437665 [details]
Patch for landing

[EWS]

Committed r282174 (241466@main): <https://commits.webkit.org/241466@main>

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