Bug 168126

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.

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

Looks like a good idea. But we still need to find a way to close the WebPage objects without crashing.

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

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.

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

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

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.

This is not specific to GTK+ port

(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

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

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

Created attachment 322376 [details] Patch Let's try this.

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

Comment on attachment 322376 [details] 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

Committed r222772: <http://trac.webkit.org/changeset/222772>

BTW, it should go without saying, but since we know to expect regressions, *please* do not backport this to 2.18.