Bug 184445 – [GTK][WPE] Race condition when destroying webprocesses

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Miguel Gomez]

When a WebPageProxy gets destroyed, this is that ideally happens:

* WebPageProxy::close() is called:
  * Sends a WebPage::Close() message to the webprocess, so it can properly close its page before shutting down the process
  * Calls WebProcessProxy::removeWebPage() which, in the end, closes the IPC connection to the webprocess
* At the same time, when the webprocess executes WebPage::close() in response to the WebPage::Close() message
  * releases the page resources
  * at the end calls WebProcess::removeWebPage(), which causes the web process termination


But there's the possibility that the webprocess never processes the WebPage::Close() message. This happens when it detects that the IPC connection has been closed before processing the message. In that case, the webprocess exits without properly closing its webpages, causing problems on situations where a proper deinitialization is required (for example deinitializing a hardware decoder used by the media player).

We need to ensure that the WebPage instances owned by the WebProcess are always properly closed before exiting.

[Miguel Gomez]

One option to fix this could be making the WebPage::Close() message a sync one, and make the ui process wait for the web process to properly close before shutting down, but this would delay closing the browser and that is a problem for the Apple ports.

So, if we cannot wait for the web process to end, we should ensure that it will close its WebPages on exit. The easiest way is to use the code that does that on debug inside WebProcess::didClose().

[Carlos Garcia Campos]

(In reply to Miguel Gomez from comment #1)
> One option to fix this could be making the WebPage::Close() message a sync
> one, and make the ui process wait for the web process to properly close
> before shutting down, but this would delay closing the browser and that is a
> problem for the Apple ports.
> 
> So, if we cannot wait for the web process to end, we should ensure that it
> will close its WebPages on exit. The easiest way is to use the code that
> does that on debug inside WebProcess::didClose().

Yes, I would move that part out of the debug ifdef, at least for GTK and WPE.

[Miguel Gomez]

Created attachment 337601 [details]
Patch

[Miguel Gomez]

Created attachment 337603 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 337603 [details]
Patch

Ok, let's try with this.

[WebKit Commit Bot]

Comment on attachment 337603 [details]
Patch

Clearing flags on attachment: 337603

Committed r230481: <https://trac.webkit.org/changeset/230481>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Michael Catanzaro]

IMO this is wrong, this code should not be platform-specific. If it's not good enough for Apple, why should it be good enough for us?

The web process is *expected* to crash, and WebKit is designed around that expectation. It must not be expected to release any system resources, or your application is going to be in big trouble when it does crash. Sounds like you need to find some other solution to handle these hardware media decoders, if they are not robust to process crashes.

(In reply to Miguel Gomez from comment #1)
> One option to fix this could be making the WebPage::Close() message a sync
> one, and make the ui process wait for the web process to properly close
> before shutting down, but this would delay closing the browser and that is a
> problem for the Apple ports.

That would be a problem for us too, because we know that in practice the web process often hangs on exit. So let's not try that, either.