[Webkit-unassigned] [Bug 183348] REGRESSION(r222772): [GTK][WPE] WebProcess from WebKitGtk+ 2.19.9x SIGSEVs in WebKit::WebProcess::ensureNetworkProcessConnection() at Source/WebKit/WebProcess/WebProcess.cpp:1127

Fri Apr 27 01:07:52 PDT 2018

https://bugs.webkit.org/show_bug.cgi?id=183348

--- Comment #45 from Carlos Garcia Campos <cgarcia at igalia.com> ---
(In reply to Alex Christensen from comment #44)
> I don't think it's a good design to use a bool that is only set in
> platform-specific code for this sort of thing.  It gives no insight into why
> we should exit on sync message sending failure.  For example, it makes me
> wonder why it is only set in the WebProcess.  Don't the other processes need
> it for the same reason, or do we just not set it there because we haven't
> observed many crashes there?  What is the underlying reason why we need to
> exit?  It also looks platform-independent when it is not.

Well, the reason why setShouldExitOnSyncMessageSendFailure() exists is probably out of scope of this bug, but I'll try to explain:

 - It was initially added in r78392 to fix a crash when the UI process closes the connection while the web process was handling a message. At that time (2011) only web and plugin process existed and both were implementing the connection client method to exit(0) with these comments:

// We were making a synchronous call to a web process that doesn't exist any more.
// Callers are unlikely to be prepared for an error like this, so it's best to exit immediately.

// We were making a synchronous call to a UI process that doesn't exist any more.
// Callers are unlikely to be prepared for an error like this, so it's best to exit immediately.

 - Then in r84288 (still 2011) the connection client method was changed to a simple flag (bool), the plugin process implementation was removed but not replaced with a call to setShouldExitOnSyncMessageSendFailure(true) like the web process. I don't know why because neither the changelog nor the bug report says why.

 - New processes were added and none of them call setShouldExitOnSyncMessageSendFailure(true). This probably proves that callers are indeed prepared to fail on send sync message, since all process are handling them (and now also the web process in GTK and WPE).

 - This behavior has always been a headache for me, I've had to debug the web process termination several times to fix different bugs and the inconsistent non-deterministic way of terminating the web process has always made it painful. But not only because of that, but because we have needed a proper termination of the web process multiple times in the past (to save the clipboard contents, to dump the old soup disk cache, and some other things we needed to to on shutdown).

 - I tried to fix it once and for all in bug #147036 (2015) but Darin opposed to it because that might slow down the web process termination (I agree it's important that a tab closes immediately, but that's still the case for us). The bug was closed as WONTFIX and I tried to workaround all the issues we had related tot that.

 - But new issues appeared related to the web process exiting on sync message failure. And I finally got an approval to not exit on sync message failure for GTK and WPE in bug #168126 (2018). Brady said that the change would slow down browser quitting when there are many tabs, and that it's not desirable for the platform that don't really needed it. And I agree, that's what makes this change platform specific, it's an optimization for platforms where all resources are guaranteed to be released no matter how the process is terminated. That's unfortunately not our case, and it's also probably not WebKit faults, but it's so easy to fix. 

 - So, after years I finally go the approval and landed the patch. WebKitGTK+ 2.20 was released with the patch and nobody has complained of browser being slow to close, I hadn't notice any slow down and I always have a lot of tabs open. So, I think this was the right fix to ensure resources are released and makes web process termination consistent with all other processes and deterministic.

 - But this bug showed up, because the existing code assumes that the web process always exits on sync message failure. But that's no longer the case for us. I fixed this bug in 2.20.1 and nobody has complained either.

 - So, this is not platform specific, but we decided to use a different behavior for GTK and WPE, because we need it, and to make sure other platforms that don't need it are not affected. I would be more than happy if we simply remove setShouldExitOnSyncMessageSendFailure(), but I don't know if that will have any implication in iOS/Mac.

Sorry for the long comment, I hope it helps.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20180427/e9eb9477/attachment.html>