Bug 235224 – [GTK][WPE] REGRESSION(r264408): Exit cleanly if Connection to UIProcess severed

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

[Pablo Saavedra]

Related-to: https://bugs.webkit.org/show_bug.cgi?id=217655#c14

I've noticed that this commit (https://bugs.webkit.org/show_bug.cgi?id=214307) have deleted some specific logic that affects to the WPE and GTK ports:


```
commit 813abc09e84c9a406836319bfa8f7fb83d378605
Author: [e-mail] <[e-mail]@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Date:   Wed Jul 15 18:23:22 2020 +0000

    [WK2] Make establishing a connection between the WebProcess and the Network process more robust
    https://bugs.webkit.org/show_bug.cgi?id=214307
    <rdar://problem/64592340>
    
    Reviewed by Alex Christensen.
```


``` diff
diff --git a/Source/WebKit/WebProcess/WebProcess.cpp b/Source/WebKit/WebProcess/WebProcess.cpp
index 6f2d8fad165f..c73c43e9857c 100644
--- a/Source/WebKit/WebProcess/WebProcess.cpp
+++ b/Source/WebKit/WebProcess/WebProcess.cpp
@@ -1100,23 +1100,28 @@ void WebProcess::setInjectedBundleParameters(const IPC::DataReference& value)
 static NetworkProcessConnectionInfo getNetworkProcessConnection(IPC::Connection& connection)
 {
     NetworkProcessConnectionInfo connectionInfo;
-    if (!connection.sendSync(Messages::WebProcessProxy::GetNetworkProcessConnection(), Messages::WebProcessProxy::GetNetworkProcessConnection::Reply(connectionInfo), 0)) {
-        // If we failed the first time, retry once. The attachment may have become invalid
-        // before it was received by the web process if the network process crashed.
-        if (!connection.sendSync(Messages::WebProcessProxy::GetNetworkProcessConnection(), Messages::WebProcessProxy::GetNetworkProcessConnection::Reply(connectionInfo), 0)) {
-#if PLATFORM(GTK) || PLATFORM(WPE)
-            // GTK+ and WPE ports don't exit on send sync message failure.
-            // In this particular case, the network process can be terminated by the UI process while the
-            // Web process is still initializing, so we always want to exit instead of crashing. This can
-            // happen when the WebView is created and then destroyed quickly.
-            // See https://bugs.webkit.org/show_bug.cgi?id=183348.
+    auto requestConnection = [&] {
+        if (!connection.isValid()) {
+            // Connection to UIProcess has been severed, exit cleanly.
             exit(0);
-#else
-            CRASH();
-#endif
...
```

The change removes the specific logic for WPE and GTK added in:

```
commit f0627f5287186ec975c3439e9bcfe119493adc95
Author: [e-mail] <[e-mail]@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Date:   Thu May 3 08:45:32 2018 +0000

    REGRESSION(r222772): [GTK][WPE] WebProcess from WebKitGtk+ 2.19.9x SIGSEVs in WebKit::WebProcess::ensureNetworkProcessConnection() at Source/WebKit/WebProcess/WebProcess.cpp:1127
    https://bugs.webkit.org/show_bug.cgi?id=183348
```


The comment in https://bugs.webkit.org/show_bug.cgi?id=183348#c40 offers a justification for that code.

[Pablo Saavedra]

Created attachment 449152 [details]
patch

[Carlos Garcia Campos]

Comment on attachment 449152 [details]
patch

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

> Source/WebKit/ChangeLog:8
> +        This patch reverts the WPE and GTK specific code removed in '[WK2] Make
> +        establishing a connection between the WebProcess and the Network process more
> +        robust' (https://bugs.webkit.org/show_bug.cgi?id=214307):

Better use the revision number to refer to other commits r12345.

> Source/WebKit/ChangeLog:10
> +        GTK+ and WPE ports don't exit on send sync message failure. In those particular

GTK+ -> GTK

> Source/WebKit/ChangeLog:14
> +        cases, the network process can be terminated by the UI process while the
> +        Web process is still initializing, so we always want to exit instead of crashing. This can
> +        happen when the WebView is created and then destroyed quickly.
> +        See https://bugs.webkit.org/show_bug.cgi?id=183348.

Have you confirmed this is still a problem?

> Source/WebKit/WebProcess/WebProcess.cpp:1123
> +            // GTK+ and WPE ports don't exit on send sync message failure.

GTK+ -> GTK

> Source/WebKit/WebProcess/WebProcess.cpp:1284
> +            // GTK+ and WPE ports don't exit on send sync message failure.

Ditto.

> Source/WebKit/WebProcess/WebProcess.cpp:1378
> +            // GTK+ and WPE ports don't exit on send sync message failure.

Ditto.

> Source/WebKit/WebProcess/WebProcess.cpp:1386
> +#endif

Too much duplicated code, maybe we can add a function failedToSendSyncMessage or something like that and move this logic there.

[Pablo Saavedra]

Created attachment 449796 [details]
patch

[Pablo Saavedra]

(In reply to Carlos Garcia Campos from comment #2)
> Comment on attachment 449152 [details]
> patch
...
> GTK+ -> GTK
> 
> > Source/WebKit/ChangeLog:14
> > +        cases, the network process can be terminated by the UI process while the
> > +        Web process is still initializing, so we always want to exit instead of crashing. This can
> > +        happen when the WebView is created and then destroyed quickly.
> > +        See https://bugs.webkit.org/show_bug.cgi?id=183348.
> 
> Have you confirmed this is still a problem?
> 


We observed this problem in custom WPE browser what makes extensive use of uses PSON.

[Pablo Saavedra]

Created attachment 449802 [details]
patch

[Michael Catanzaro]

Comment on attachment 449802 [details]
patch

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

> Source/WebKit/WebProcess/WebProcess.cpp:1104
> +    // GTK and WPE ports don't exit on send sync message failure.

Hm, this comment is wrong. You clearly do exit right below.

I see the comment originally comes from r231298, but the context has changed a little since then: now you've got this in its own function called "failedToSendSyncMessage," and saying we don't exit on sync message failure and then doing exactly that doesn't make sense. Needs to be reworded... somehow. Ideally, we'd explain why it's OK for GTK/WPE to exit cleanly while other ports crash.

It might make sense to remove the PLATFORM guards here, btw. Looking at the context in https://bugs.webkit.org/show_bug.cgi?id=183348#c45, I see they were added to resolve an emergency when we didn't have time to convince Apple owners that the code is cleaner without them. This might be a good time to try again, because it's really confusing why some ports want to crash here and others don't. At the least, we should try to indicate why different ports do different things in the comment. I wonder what Carlos Garcia thinks about this, since he's dealt with most of the web process termination bugs over the years.

[Pablo Saavedra]

(In reply to Michael Catanzaro from comment #6)
> Comment on attachment 449802 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=449802&action=review
> 
> > Source/WebKit/WebProcess/WebProcess.cpp:1104
> > +    // GTK and WPE ports don't exit on send sync message failure.
> 
> Hm, this comment is wrong. You clearly do exit right below.
> 
> I see the comment originally comes from r231298, but the context has changed
> a little since then: now you've got this in its own function called
> "failedToSendSyncMessage," and saying we don't exit on sync message failure
> and then doing exactly that doesn't make sense. Needs to be reworded...
> somehow. Ideally, we'd explain why it's OK for GTK/WPE to exit cleanly while
> other ports crash.
> 
> It might make sense to remove the PLATFORM guards here, btw. Looking at the
> context in https://bugs.webkit.org/show_bug.cgi?id=183348#c45, I see they
> were added to resolve an emergency when we didn't have time to convince
> Apple owners that the code is cleaner without them. This might be a good
> time to try again, because it's really confusing why some ports want to
> crash here and others don't. At the least, we should try to indicate why
> different ports do different things in the comment. I wonder what Carlos
> Garcia thinks about this, since he's dealt with most of the web process
> termination bugs over the years.

I see your point. This change is too much delicate. I agree with you, let's wait for the Carlos's input. He could have another ideas or he could suggest another approach for address this.

[Carlos Garcia Campos]

Comment on attachment 449802 [details]
patch

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

> Source/WebKit/ChangeLog:3
> +        [GTK][WPE] Exit cleanly if Connection to UIProcess severed. Regression (r214307)

Please, make this match the bug title, note that 214307 is not the revision number, but the bug number.

> Source/WebKit/ChangeLog:8
> +        robust' (r12345):

12345 was an example, it should be r264408.

> Source/WebKit/ChangeLog:10
> +        GTK+ and WPE ports don't exit on send sync message failure. In those particular

GTK+ -> GTK

> Source/WebKit/ChangeLog:14
> +        See https://bugs.webkit.org/show_bug.cgi?id=183348.

The patch landed in this bug included a unit test that is still passing. Are you sure this is still a problem? We would need  a way to reproduce the crash or a test.

>>> Source/WebKit/WebProcess/WebProcess.cpp:1104
>>> +    // GTK and WPE ports don't exit on send sync message failure.
>> 
>> Hm, this comment is wrong. You clearly do exit right below.
>> 
>> I see the comment originally comes from r231298, but the context has changed a little since then: now you've got this in its own function called "failedToSendSyncMessage," and saying we don't exit on sync message failure and then doing exactly that doesn't make sense. Needs to be reworded... somehow. Ideally, we'd explain why it's OK for GTK/WPE to exit cleanly while other ports crash.
>> 
>> It might make sense to remove the PLATFORM guards here, btw. Looking at the context in https://bugs.webkit.org/show_bug.cgi?id=183348#c45, I see they were added to resolve an emergency when we didn't have time to convince Apple owners that the code is cleaner without them. This might be a good time to try again, because it's really confusing why some ports want to crash here and others don't. At the least, we should try to indicate why different ports do different things in the comment. I wonder what Carlos Garcia thinks about this, since he's dealt with most of the web process termination bugs over the years.
> 
> I see your point. This change is too much delicate. I agree with you, let's wait for the Carlos's input. He could have another ideas or he could suggest another approach for address this.

I see the comment is confusing, it actually means that GTK and WPE don't exit on send sync message failure in general (the setting is disabled), but in this particular case we want to exit, but not crash.

[Pablo Saavedra]

Still I'm not sure this is the same case than the one described in the test case but I just got a crash. Here the trace:

```
#0  __libc_do_syscall () at libc-do-syscall.S:49
#1  0x74a4ca56 in __libc_signal_restore_set (set=0x7e90aac8) at ../sysdeps/unix/sysv/linux/internal-signals.h:86
#2  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:48
#3  0x74a3d768 in __GI_abort () at abort.c:79
#4  0x75190af6 in WebKit::WebProcess::ensureNetworkProcessConnection() () from /home/psaavedra/Downloads/webkit/usr/lib/libWPEWebKit-1.0.so.3.11.9
#5  0x7518ffd0 in WebKit::WebProcess::initializeWebProcess(WebKit::WebProcessCreationParameters&&) () from /home/psaavedra/Downloads/webkit/usr/lib/libWPEWebKit-1.0.so.3.11.9
#6  0x74ee804c in WebKit::WebProcess::didReceiveWebProcessMessage(IPC::Connection&, IPC::Decoder&) () from /home/psaavedra/Downloads/webkit/usr/lib/libWPEWebKit-1.0.so.3.11.9
#7  0x75007d18 in IPC::Connection::dispatchMessage(std::unique_ptr<IPC::Decoder, std::default_delete<IPC::Decoder> >) () from /home/psaavedra/Downloads/webkit/usr/lib/libWPEWebKit-1.0.so.3.11.9
#8  0x75007f96 in IPC::Connection::dispatchOneIncomingMessage() () from /home/psaavedra/Downloads/webkit/usr/lib/libWPEWebKit-1.0.so.3.11.9
#9  0x7694d280 in WTF::RunLoop::performWork() () from /home/psaavedra/Downloads/webkit/usr/lib/libWPEWebKit-1.0.so.3.11.9
#10 0x76986ad8 in WTF::RunLoop::TimerBase::TimerBase(WTF::RunLoop&)::$_3::__invoke(void*) () from /home/psaavedra/Downloads/webkit/usr/lib/libWPEWebKit-1.0.so.3.11.9
#11 0x769861dc in WTF::RunLoop::$_0::__invoke(_GSource*, int (*)(void*), void*) () from /home/psaavedra/Downloads/webkit/usr/lib/libWPEWebKit-1.0.so.3.11.9
#12 0x7474a74e in g_main_dispatch (context=0x22498c8) at ../glib-2.62.6/glib/gmain.c:3216
#13 g_main_context_dispatch (context=context@entry=0x22498c8) at ../glib-2.62.6/glib/gmain.c:3908
#14 0x7474a9e4 in g_main_context_iterate (context=0x22498c8, block=block@entry=1, dispatch=dispatch@entry=1, self=<optimized out>) at ../glib-2.62.6/glib/gmain.c:3981
#15 0x7474ac50 in g_main_loop_run (loop=0x224ae58) at ../glib-2.62.6/glib/gmain.c:4175
#16 0x76986620 in WTF::RunLoop::run() () from /home/psaavedra/Downloads/webkit/usr/lib/libWPEWebKit-1.0.so.3.11.9
#17 0x75231ee6 in int WebKit::AuxiliaryProcessMain<WebKit::WebProcess, WebKit::WebProcessMainWPE>(int, char**) () from /home/psaavedra/Downloads/webkit/usr/lib/libWPEWebKit-1.0.so.3.11.9
#18 0x74a3d9fa in __libc_start_main (main=0x4e7fd4, argc=0, argv=0x0, init=<optimized out>, fini=0x4e678d <__libc_csu_fini>, rtld_fini=0x76f7b02d <_dl_fini>, stack_end=0x7e90b4f4) at libc-start.c:308
#19 0x004e6608 in _start () at start.S:112
```

[Carlos Garcia Campos]

*** Bug 238325 has been marked as a duplicate of this bug. ***

[Carlos Garcia Campos]

Comment on attachment 449802 [details]
patch

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

> Source/WebKit/WebProcess/WebProcess.cpp:1101
> +[[ noreturn ]] inline void failedToSendSyncMessage()

NO_RETURN

[Pablo Saavedra]

Created attachment 456004 [details]
patch

[Michael Catanzaro]

(In reply to Pablo Saavedra from comment #12)
> Created attachment 456004 [details]
> patch

Hi Pablo, is it OK for us to land this?

[EWS]

Committed r292099 (249019@main): <https://commits.webkit.org/249019@main>

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

[Radar WebKit Bug Importer]

<rdar://problem/91030061>