Bug 224533

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.

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

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

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.

Created attachment 427352 [details] Patch

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

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.

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

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

Created attachment 427401 [details] Patch

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.

Created attachment 427403 [details] Patch

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

Created attachment 427411 [details] Patch

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

WEBKIT_WEB_PROCESS_TERMINATED_BY_API

Created attachment 427419 [details] Patch

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

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