Bug 224533 – [GTK][WPE] Properly recover from unresponsive web processes

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

[Miguel Gomez]

Bug 224359 added a signal to WebKitWebView to indicate whether the web process associated to it has become unresponsive. But currently the only way to recover from that scenario is by recreating the view, as the other methods that could be useful (reload, uri or load alternate html) will hang if the web process is not responsive. We need to look into those to make them work in that scenario.

[Fujii Hironori]

You can get the pid of the web process by using WKPageGetProcessIdentifier API. You can kill the unresponsive web process. I am not sure this is the proper way to recover.

[Miguel Gomez]

(In reply to Fujii Hironori from comment #1)
> You can get the pid of the web process by using WKPageGetProcessIdentifier
> API. You can kill the unresponsive web process. I am not sure this is the
> proper way to recover.

I think that's the intended way to do it when using the C API (I know some people are doing it that way).
In the glib API we don't have an equivalent to WKPageGetProcessIdentifier though. We could add one and follow the same approach, or we could just add an API method to WebKitWebView to directly kill the web content process.

[Michael Catanzaro]

(In reply to Miguel Gomez from comment #2)
> or we could just add
> an API method to WebKitWebView to directly kill the web content process.

That seems like a better approach, but I'm not convinced it's needed. webkit_web_view_load_* could do the killing for us internally.

[Carlos Garcia Campos]

My initial idea was to implicitly kill the web process if it's unresponsive when new load happens or on refresh, but now I'm more convinced that an explicit approach would be better. It might not be obvious for the user that a new load needs to happen, for example. In the case of a crash we don't have a web process anymore, so a new load is no different than the initial load. And we probably only want to kill the process if the user handled the signal, but it's a property notification so we don't really know what the user replied. If we decide to go with the explicit approach I would expose a method to kill the web view web process, I wouldn't expose process ids.

[Miguel Gomez]

Created attachment 427352 [details]
Patch

[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 https://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Michael Catanzaro]

Comment on attachment 427352 [details]
Patch

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

LGTM

> Source/WebKit/UIProcess/API/glib/WebKitNavigationClient.cpp:121
> +        case ProcessTerminationReason::ExceededCPULimit:

Preexisting problem: this one should use WEBKIT_WEB_PROCESS_CRASHED (or ideally get its own public enum value). The other three cases occur when the process is terminated due to implementation detail, so it makes sense that the application is not notified. But exceeded CPU limit is definitely something the application needs to know about in order to offer the process crash error page.

> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:4780
> +    if (auto* provisionalPageProxy = page.provisionalPageProxy()) {
> +        Ref<WebKit::WebProcessProxy> protectedProcessProxy(provisionalPageProxy->process());
> +        protectedProcessProxy->requestTermination(WebKit::ProcessTerminationReason::RequestedByClient);
> +    }

Hmm... do we really want to kill the provisional page? I'm not entirely sure what the provisional page is, but my guess is that is the prewarmed process that is waiting to be swapped in. If so, we don't want to kill it, as it's not unresponsive, and that would slow down the process swap that's about to occur. Am I wrong?

> Source/WebKit/UIProcess/API/gtk/WebKitWebView.h:196
> + * @WEBKIT_WEB_PROCESS_REQUESTED_BY_API: the web process termination was requested by an API call.

@WEBKIT_WEB_PROCESS_REQUESTED_BY_API: the web process termination was requested by an API call. Since: 2.34

> Source/WebKit/UIProcess/API/wpe/WebKitWebView.h:165
> + * @WEBKIT_WEB_PROCESS_REQUESTED_BY_API: the web process termination was requested by an API call.

@WEBKIT_WEB_PROCESS_REQUESTED_BY_API: the web process termination was requested by an API call. Since: 2.34

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitWebView.cpp:1551
> +    static gboolean webProcessTerminatedCallback(WebKitWebView *webView, WebKitWebProcessTerminationReason reason, WebViewTerminateWebProcessTest* test)

This should return void, not gboolean, since the web-process-terminated signal has no return value.

Also the * is hanging wrong: WebKitWebView* webView

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitWebView.cpp:1586
> +        "   setTimeout(function() {"
> +        "    var start = new Date().getTime();"
> +        "    var end = start;"
> +        "     while(end < start + 4000) {"
> +        "      end = new Date().getTime();"
> +        "     }"
> +        "    }, 500);"

I think you could just do while(true) in the timeout callback.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #7)
> Hmm... do we really want to kill the provisional page? I'm not entirely sure
> what the provisional page is, but my guess is that is the prewarmed process
> that is waiting to be swapped in. If so, we don't want to kill it, as it's
> not unresponsive, and that would slow down the process swap that's about to
> occur. Am I wrong?

Hm, maybe it is a page that's used during provisional loads before the load is committed? Then killing it is needed in case it is unresponsive?

[Carlos Garcia Campos]

Comment on attachment 427352 [details]
Patch

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

>> Source/WebKit/UIProcess/API/glib/WebKitNavigationClient.cpp:121
>> +        case ProcessTerminationReason::ExceededCPULimit:
> 
> Preexisting problem: this one should use WEBKIT_WEB_PROCESS_CRASHED (or ideally get its own public enum value). The other three cases occur when the process is terminated due to implementation detail, so it makes sense that the application is not notified. But exceeded CPU limit is definitely something the application needs to know about in order to offer the process crash error page.

We don't have CPU limit enabled, only memory limit. I agree we should enable it and then expose the option, but that should be a separate bug.

>> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:4780
>> +    }
> 
> Hmm... do we really want to kill the provisional page? I'm not entirely sure what the provisional page is, but my guess is that is the prewarmed process that is waiting to be swapped in. If so, we don't want to kill it, as it's not unresponsive, and that would slow down the process swap that's about to occur. Am I wrong?

This is what cocoa does

[Miguel Gomez]

Created attachment 427401 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 427401 [details]
Patch

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

> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:4763
> + * signal is emitted with WEBKIT_WEB_PROCESS_REQUESTED_BY_API as the reason for

WEBKIT_WEB_PROCESS_REQUESTED_BY_API -> %WEBKIT_WEB_PROCESS_REQUESTED_BY_API

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitWebView.cpp:1561
> +    g_signal_connect_after(test->m_webView, "web-process-terminated", G_CALLBACK(WebViewTerminateWebProcessTest::webProcessTerminatedCallback), test);

I would connect to the signal from the WebViewTerminateWebProcessTest constructor

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitWebView.cpp:1570
> +    g_signal_handlers_disconnect_by_func(test->m_webView, reinterpret_cast<void*>(WebViewTerminateWebProcessTest::webProcessTerminatedCallback), test);

And disconnect from the destructor using g_signal_handlers_disconnect_by_data

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitWebView.cpp:1586
> +    g_signal_connect_after(test->m_webView, "web-process-terminated", G_CALLBACK(WebViewTerminateWebProcessTest::webProcessTerminatedCallback), test);

Ditto.

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitWebView.cpp:1606
> +    g_signal_handlers_disconnect_by_func(test->m_webView, reinterpret_cast<void*>(WebViewTerminateWebProcessTest::webProcessTerminatedCallback), test);

Ditto.

[Miguel Gomez]

Created attachment 427403 [details]
Patch

[Michael Catanzaro]

Comment on attachment 427403 [details]
Patch

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

OK, nice.

> Source/WebKit/UIProcess/API/gtk/WebKitWebView.h:196
> + * @WEBKIT_WEB_PROCESS_REQUESTED_BY_API: the web process termination was requested by an API call. Since: 2.34

Let's call it WEBKIT_WEB_PROCESS_TERMINATION_REQUESTED_BY_API or WEBKIT_WEB_PROCESS_KILL_REQUESTED_BY_API

[Miguel Gomez]

Created attachment 427411 [details]
Patch

[Adrian Perez]

(In reply to Michael Catanzaro from comment #13)
> Comment on attachment 427403 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=427403&action=review
> 
> OK, nice.
> 
> > Source/WebKit/UIProcess/API/gtk/WebKitWebView.h:196
> > + * @WEBKIT_WEB_PROCESS_REQUESTED_BY_API: the web process termination was requested by an API call. Since: 2.34
> 
> Let's call it WEBKIT_WEB_PROCESS_TERMINATION_REQUESTED_BY_API or
> WEBKIT_WEB_PROCESS_KILL_REQUESTED_BY_API

Gah! Such a mouthful. How about WEBKIT_WEB_PROCESS_API_TERMINATION,
which is two words less? :)

[Carlos Garcia Campos]

WEBKIT_WEB_PROCESS_TERMINATED_BY_API

[Miguel Gomez]

Created attachment 427419 [details]
Patch

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #16)
> WEBKIT_WEB_PROCESS_TERMINATED_BY_API

Good name.

[EWS]

Committed r276848 (237199@main): <https://commits.webkit.org/237199@main>

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