168126 2017-02-10 10:23:14 -0800 [GTK][WPE] WebProcess should run cleanup on quit to release resources 2018-04-09 13:08:34 -0700 1 1 1 Unclassified WebKit WebKitGTK Other Unspecified Unspecified RESOLVED FIXED https://bugs.webkit.org/show_bug.cgi?id=166029 https://bugs.webkit.org/show_bug.cgi?id=146657 https://bugs.webkit.org/show_bug.cgi?id=147036 https://bugs.webkit.org/show_bug.cgi?id=183348 P2 Normal --- 1 olivier.blin webkit-unassigned annulen beidson bugs-noreply cgarcia mcatanzaro zan oldest_to_newest 1275367 0 olivier.blin 2017-02-10 10:23:14 -0800 When the UI Process is closed, the WebProcess does not always run a proper cleanup, and may not properly close system resources. For example, the MediaPlayerPrivateGStreamerBase destructor may not be called. This can be especially annoying on embedded devices, where video drivers do not always run a device cleanup when the process using it is terminated. This has been seen initially on WebKitForWayland with GStreamer, and WebKitGTK has the same behavior. When exiting MiniBrowser by closing the graphical window, WebPage::close() is called on the WebProcess, and it leads most of the time to a successful call of ~MediaPlayerPrivateGStreamerBase(). When exiting MiniBrowser from command line with Ctrl-C, SIGINT is sent to all process in the process group, and WebProcess quits without properly destroying its objects. When exiting MiniBrowser with killall (SIGTERM), the WebProcess also quits without destroying its objects. 1275394 1 olivier.blin 2017-02-10 10:41:03 -0800 We could catch SIGINT in the WebProcess, as discussed on IRC with Zan and Konstantin a few months ago, but there is no ideal method to call to run the WebProcess exit. If we add a SIGINT handler and do nothing more, the IPC connection close will eventually be detected. But in release mode, WebProcess::didClose() does not run the WebPage cleanup. After enabling the code that runs WebPage::close() in WebProcess::didClose(), the WebProcess will exit even earlier because of extra IPC failure checks: #0 __GI_exit (status=0) at exit.c:104 #1 0x00007f914bceda79 in IPC::Connection::sendSyncMessage(unsigned long, std::unique_ptr<IPC::Encoder, std::default_delete<IPC::Encoder> >, WTF::Seconds, WTF::OptionSet<IPC::SendSyncOption>) () from libwebkit2gtk-4.0.so.37 #2 0x00007f914bdf35fb in bool IPC::Connection::sendSync<Messages::WebProcessProxy::ShouldTerminate>(Messages::WebProcessProxy::ShouldTerminate&&, Messages::WebProcessProxy::ShouldTerminate::Reply&&, unsigned long, WTF::Seconds, WTF::OptionSet<IPC::SendSyncOption>) () from libwebkit2gtk-4.0.so.37 #3 0x00007f914bdedec8 in WebKit::WebProcess::shouldTerminate() () from libwebkit2gtk-4.0.so.37 #4 0x00007f914bcfc4e3 in WebKit::ChildProcess::enableTermination() () from libwebkit2gtk-4.0.so.37 #5 0x00007f914be76da0 in WebKit::WebPage::close() () from libwebkit2gtk-4.0.so.37 #6 0x00007f914bdee178 in WebKit::WebProcess::didClose(IPC::Connection&) () from libwebkit2gtk-4.0.so.37 And if we call WebProcess::singleton().parentProcessConnection()->setShouldExitOnSyncMessageSendFailure(false) in the SIGINT handler, this leads to different crashes later on. 1275409 2 annulen 2017-02-10 10:55:55 -0800 It might be a better idea to add new IPC message that instructs WebProcess to shutdown. FWIW, there were requests to make graceful restart of WebProcess possible, and having this message would be useful for both cases 1275416 3 olivier.blin 2017-02-10 11:02:02 -0800 Looks like a good idea. But we still need to find a way to close the WebPage objects without crashing. 1275734 4 mcatanzaro 2017-02-10 20:23:42 -0800 Just to be clear, this is definitely a bug in the video drivers, right? Not releasing system resources when the process quits is pretty wild.... 1275762 5 cgarcia 2017-02-10 23:13:16 -0800 This is not specific to the web process, though. We have similar issues with the network process because the soup session is not properly finished. See bug #166029. 1275870 6 olivier.blin 2017-02-11 15:38:25 -0800 (In reply to comment #4) > Just to be clear, this is definitely a bug in the video drivers, right? Not > releasing system resources when the process quits is pretty wild.... Yes, it is a bug in the video drivers. Unfortunately, this is not uncommon in proprietary drivers for embedded devices. It would be good to have a graceful WebProcess exit. Is there any reason not to do so? 1276216 7 zan 2017-02-13 09:41:56 -0800 (In reply to comment #0) > When exiting MiniBrowser by closing the graphical window, WebPage::close() > is called on the WebProcess, and it leads most of the time to a successful > call of ~MediaPlayerPrivateGStreamerBase(). This should always be the preferred path. To what degree things can be uninitialized is to be seen I guess. We've had issues with graphics drivers and atexit handlers before. > When exiting MiniBrowser from command line with Ctrl-C, SIGINT is sent to > all process in the process group, and WebProcess quits without properly > destroying its objects. > When exiting MiniBrowser with killall (SIGTERM), the WebProcess also quits > without destroying its objects. This can get tricky. Ideally WebProcess would only be closed when the UIProcess signals it to do so. Sending SIGINT or SIGTERM directly at a WebProcess or NetworkProcess should also kill it. But Ctrl-C is a bit special since it's super useful for development purposes, yet can't be worked around easily since it sends out SIGINT to all the processes in the process group, and AFAIU if we don't handle SIGINT in the WebProcess, SIGINT will kill that WebProcess before the UIProcess can shut it down when it handles its own SIGINT. 1276231 8 mcatanzaro 2017-02-13 10:29:12 -0800 As a rule, I'm opposed to adding workarounds to WebKit for bugs in crap proprietary software. But in this case it seems like the WebKit behavior should be improved regardless. Our secondary processes should probably behave similarly to how I decided to handle this in Epiphany: * First time a shutdown signal (SIGINT or SIGTERM) is received, attempt to quit normally, the same way we would if attempting to close the graphical window. It may fail. * Second time a shutdown signal is received, quit immediately. Useful in case something goes wrong in step one. Note: I think it's important for GLib ports to catch the shutdown signals using g_unix_signal_add() rather than the low-level POSIX APIs, in order to avoid the massive amount of problems and races with UNIX signal handling and async signal safety. In Epiphany, I implemented the above by connecting the signal source once and always disconnecting it when received, so that receiving the signal subsequently would trigger the default killer handler. Note that it doesn't always work because sometimes the signal source handler gets stuck running forever; I never figured out why. Note: In any case, *always* end with re-raising the caught signal, so that the process exit status indicates the signal that was received. 1335260 9 cgarcia 2017-08-04 08:36:33 -0700 This is not specific to GTK+ port 1335946 10 beidson 2017-08-07 07:28:10 -0700 (In reply to Olivier Blin from comment #6) > (In reply to comment #4) > > Just to be clear, this is definitely a bug in the video drivers, right? Not > > releasing system resources when the process quits is pretty wild.... > > Yes, it is a bug in the video drivers. > Unfortunately, this is not uncommon in proprietary drivers for embedded > devices. > > It would be good to have a graceful WebProcess exit. > Is there any reason not to do so? Yes. It will slow down things like quitting the browser with lots of tabs open. And on platforms without these issues, it will slow them unnecessarily. If this is truly needed for GTK/WPE/etc, it will need to be specific to those platforms. We don't want to slow down WebView destruction on Mac/iOS 1335954 11 mcatanzaro 2017-08-07 08:22:56 -0700 (In reply to Brady Eidson from comment #10) > If this is truly needed for GTK/WPE/etc, it will need to be specific to > those platforms. Well it depends. On Linux it is considered nice, but optional, to release system resources before process termination. The OS is responsible for handling that if the process does not. The original reported problem here seems to be a bug with some graphics driver not doing that. We definitely do not need to support broken drivers, at least not upstream. The driver needs to be fixed. Now, we really do need to finalize the SoupSession, but we already do that now (bug #166029) and that doesn't affect macOS at all. 1337875 12 cgarcia 2017-08-11 06:36:59 -0700 (In reply to Brady Eidson from comment #10) > (In reply to Olivier Blin from comment #6) > > (In reply to comment #4) > > > Just to be clear, this is definitely a bug in the video drivers, right? Not > > > releasing system resources when the process quits is pretty wild.... > > > > Yes, it is a bug in the video drivers. > > Unfortunately, this is not uncommon in proprietary drivers for embedded > > devices. > > > > It would be good to have a graceful WebProcess exit. > > Is there any reason not to do so? > > Yes. It will slow down things like quitting the browser with lots of tabs > open. And on platforms without these issues, it will slow them unnecessarily. > > If this is truly needed for GTK/WPE/etc, it will need to be specific to > those platforms. > > We don't want to slow down WebView destruction on Mac/iOS I think we could start by not calling setShouldExitOnSyncMessageSendFailure() for GTK and WPE ports, just to try. 1355236 13 322376 cgarcia 2017-10-02 08:06:35 -0700 Created attachment 322376 Patch Let's try this. 1355238 14 olivier.blin 2017-10-02 08:42:01 -0700 (In reply to Carlos Garcia Campos from comment #13) > Created attachment 322376 [details] > Patch > > Let's try this. I think I tried this, and it did not end well. I don't remember where it was crashing though. 1355283 15 322376 mcatanzaro 2017-10-02 10:22:49 -0700 Comment on attachment 322376 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=322376&action=review I'm fully expecting many horrible regressions, but let's give it a try and see. > Source/WebKit/WebProcess/WebProcess.cpp:219 > +#if !PLATFORM(GTK) && !PLAFORM(WPE) PLAFORM -> PLATFORM 1355728 16 cgarcia 2017-10-03 01:12:29 -0700 Committed r222772: <http://trac.webkit.org/changeset/222772> 1355751 17 mcatanzaro 2017-10-03 02:28:01 -0700 BTW, it should go without saying, but since we know to expect regressions, *please* do not backport this to 2.18. 322376 2017-10-02 08:06:35 -0700 2017-10-02 10:22:49 -0700 Patch wk2-exit-on-sync-failure.diff text/plain 1274 cgarcia ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nIGIvU291cmNlL1dlYktpdC9DaGFu Z2VMb2cKaW5kZXggZWE2ZTQyZjM1ZmYuLjVmNzZiMDI4NWNlIDEwMDY0NAotLS0gYS9Tb3VyY2Uv V2ViS2l0L0NoYW5nZUxvZworKysgYi9Tb3VyY2UvV2ViS2l0L0NoYW5nZUxvZwpAQCAtMSwzICsx LDE1IEBACisyMDE3LTEwLTAyICBDYXJsb3MgR2FyY2lhIENhbXBvcyAgPGNnYXJjaWFAaWdhbGlh LmNvbT4KKworICAgICAgICBbR1RLXVtXUEVdIFdlYlByb2Nlc3Mgc2hvdWxkIHJ1biBjbGVhbnVw IG9uIHF1aXQgdG8gcmVsZWFzZSByZXNvdXJjZXMKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtp dC5vcmcvc2hvd19idWcuY2dpP2lkPTE2ODEyNgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9E WSAoT09QUyEpLgorCisgICAgICAgIERvIG5vdCBjYWxsIGV4aXQgb24gc3luYyBtZXNzYWdlIHNl bmQgZmFpbHVyZSBmb3IgR1RLIGFuZCBXUEUgcG9ydHMuCisKKyAgICAgICAgKiBXZWJQcm9jZXNz L1dlYlByb2Nlc3MuY3BwOgorICAgICAgICAoV2ViS2l0OjpXZWJQcm9jZXNzOjppbml0aWFsaXpl Q29ubmVjdGlvbik6CisKIDIwMTctMTAtMDIgIENhcmxvcyBHYXJjaWEgQ2FtcG9zICA8Y2dhcmNp YUBpZ2FsaWEuY29tPgogCiAgICAgICAgIFtHVEtdW1dQRV0gRW5hYmxlIGludGVyYWN0aXZlIGZv cm1zIHZhbGlkYXRpb24gYnkgZGVmYXVsdApkaWZmIC0tZ2l0IGEvU291cmNlL1dlYktpdC9XZWJQ cm9jZXNzL1dlYlByb2Nlc3MuY3BwIGIvU291cmNlL1dlYktpdC9XZWJQcm9jZXNzL1dlYlByb2Nl c3MuY3BwCmluZGV4IDQ0ZWI4ODMwZmMwLi5mNzU4ZGFkOTdhMSAxMDA2NDQKLS0tIGEvU291cmNl L1dlYktpdC9XZWJQcm9jZXNzL1dlYlByb2Nlc3MuY3BwCisrKyBiL1NvdXJjZS9XZWJLaXQvV2Vi UHJvY2Vzcy9XZWJQcm9jZXNzLmNwcApAQCAtMjE2LDcgKzIxNiw5IEBAIHZvaWQgV2ViUHJvY2Vz czo6aW5pdGlhbGl6ZUNvbm5lY3Rpb24oSVBDOjpDb25uZWN0aW9uKiBjb25uZWN0aW9uKQogewog ICAgIENoaWxkUHJvY2Vzczo6aW5pdGlhbGl6ZUNvbm5lY3Rpb24oY29ubmVjdGlvbik7CiAKKyNp ZiAhUExBVEZPUk0oR1RLKSAmJiAhUExBRk9STShXUEUpCiAgICAgY29ubmVjdGlvbi0+c2V0U2hv dWxkRXhpdE9uU3luY01lc3NhZ2VTZW5kRmFpbHVyZSh0cnVlKTsKKyNlbmRpZgogCiAjaWYgSEFW RShRT1NfQ0xBU1NFUykKICAgICBjb25uZWN0aW9uLT5zZXRTaG91bGRCb29zdE1haW5UaHJlYWRP blN5bmNNZXNzYWdlKHRydWUpOwo=