Bug 180862 – [GTK][WPE] Add WebKitWebView::web-process-terminated signal and deprecate web-process-crashed

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

[Carlos Garcia Campos]

Once the memory pressure web process monitor is enabled in bug #180861 the web process can be killed due to memory limits. We need to expose the reason of the web process termination to allow applications handle the memory limit case. Epiphany might want to use an error page similar to the crash one, but explaining that the process was killed due to high memory used, but other applications might want to simply reload the web process silently.

[Carlos Garcia Campos]

Created attachment 329484 [details]
Patch

WebKitWebProcessTerminationReason can be expanded to support ExceededCPULimit if we enable that.

[EWS Watchlist]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #1)
> WebKitWebProcessTerminationReason can be expanded to support
> ExceededCPULimit if we enable that.

We should definitely enable that, too.

[Michael Catanzaro]

Comment on attachment 329484 [details]
Patch

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

I thought about recommending that we adjust the API for the possibility that, in the future, we might want to notify the UI process if other secondary processes terminate. The signal could be SECONDARY_PROCESS_TERMINATED or just PROCESS_TERMINATED instead of WEB_PROCESS_TERMINATED.

But I doubt we'll ever need to do that, and this seems clearer.

> Source/WebKit/UIProcess/API/gtk/WebKitWebView.h:191
> + * @WEBKIT_WEB_PROCESS_CRAHED: the web process crashed.

CRASHED

> Source/WebKit/UIProcess/API/wpe/WebKitWebView.h:166
> +    WEBKIT_WEB_PROCESS_CRAHED,

Ditto.

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebExtensions.cpp:86
> +    g_assert_cmpuint(reason, ==, WEBKIT_WEB_PROCESS_CRAHED);

You have to look after you Ctrl+V :P

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #3)
> (In reply to Carlos Garcia Campos from comment #1)
> > WebKitWebProcessTerminationReason can be expanded to support
> > ExceededCPULimit if we enable that.
> 
> We should definitely enable that, too.

Yes, but maybe we a way to disable it.

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #4)
> Comment on attachment 329484 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=329484&action=review
> 
> I thought about recommending that we adjust the API for the possibility
> that, in the future, we might want to notify the UI process if other
> secondary processes terminate. The signal could be
> SECONDARY_PROCESS_TERMINATED or just PROCESS_TERMINATED instead of
> WEB_PROCESS_TERMINATED.

In the case of other secondary processes, for example the network process, the signal wouldn't belong to the web view, but to the web context.

> But I doubt we'll ever need to do that, and this seems clearer.
> 
> > Source/WebKit/UIProcess/API/gtk/WebKitWebView.h:191
> > + * @WEBKIT_WEB_PROCESS_CRAHED: the web process crashed.
> 
> CRASHED
> 
> > Source/WebKit/UIProcess/API/wpe/WebKitWebView.h:166
> > +    WEBKIT_WEB_PROCESS_CRAHED,
> 
> Ditto.
> 
> > Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebExtensions.cpp:86
> > +    g_assert_cmpuint(reason, ==, WEBKIT_WEB_PROCESS_CRAHED);
> 
> You have to look after you Ctrl+V :P

Indeed

[Carlos Garcia Campos]

Committed r226019: <https://trac.webkit.org/changeset/226019>