WebProcessProxy has a couple of timers that check whether web processes are responsive, but this is not communicated in any way to the WebKitWebView user. Add a property to WebKitWebView indicating so, and whose changes can be tracked by a notify.
Created attachment 425592 [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 425592 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=425592&action=review Looks good to me, but we need a test case. > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:1352 > + * Since: 2.32 2.34 > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:4770 > + * Since: 2.32 2.34
Comment on attachment 425592 [details] Patch The API addition looks good to me. Please take a look at the comments below before landing, tho. View in context: https://bugs.webkit.org/attachment.cgi?id=425592&action=review > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:814 > + priv->isWebProcessResponsive = true; This line is not needed, the default value is set to “TRUE” in the call to “g_param_spec_boolean()” inside the “webkit_web_view_class_init()” below, which will trigger setting the initial value of the property when during instantiation. > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:1360 > + _("Whether the current web process is responsive"), Nit: which process is the “current web process”? I think it would more precise to write this as “Whether the web process associated with the web view is responsive” (or similar). > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:4759 > + g_object_notify(G_OBJECT(webView), "is-web-process-responsive"); It always annoys me a tiny little bit that every time we notify of object property changes in the WebKit{GTK,WPE} code we don't use “g_object_notify_by_pspec()” to avoid the property lookup… so I filed bug #224366 to maybe do that change at some point later on 😉️ > Source/WebKit/UIProcess/API/glib/WebKitWebViewPrivate.h:123 > + Why the extra empty space? Please remove it before landing.
Comment on attachment 425592 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=425592&action=review Please, don't forget the unit tests. >> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:814 >> + priv->isWebProcessResponsive = true; > > This line is not needed, the default value is set to “TRUE” in the call to > “g_param_spec_boolean()” inside the “webkit_web_view_class_init()” below, > which will trigger setting the initial value of the property when during > instantiation. Don't we need to pass G_PARAM_CONSTRUCT flags for that?
(In reply to Carlos Garcia Campos from comment #5) > Comment on attachment 425592 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=425592&action=review > > Please, don't forget the unit tests. Sure. > >> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:814 > >> + priv->isWebProcessResponsive = true; > > > > This line is not needed, the default value is set to “TRUE” in the call to > > “g_param_spec_boolean()” inside the “webkit_web_view_class_init()” below, > > which will trigger setting the initial value of the property when during > > instantiation. > > Don't we need to pass G_PARAM_CONSTRUCT flags for that? It's been a while since I looked into gobject internals but, are you taking into account that the property doesn't have a setter? So there's no way for gobject to set the initial value to priv->isWebProcessResponsive. Unless I'm missing some magic here.
(In reply to Carlos Garcia Campos from comment #5) > Comment on attachment 425592 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=425592&action=review > > Please, don't forget the unit tests. > > >> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:814 > >> + priv->isWebProcessResponsive = true; > > > > This line is not needed, the default value is set to “TRUE” in the call to > > “g_param_spec_boolean()” inside the “webkit_web_view_class_init()” below, > > which will trigger setting the initial value of the property when during > > instantiation. > > Don't we need to pass G_PARAM_CONSTRUCT flags for that? Yes, you are right, I just checked how GObject works internally. Without G_PARAM_CONSTRUCT the default value of the GParamSpec that describes the property is never used. The GLib documentation could be more explicit about this 🤷️
Created attachment 425876 [details] Patch
Hm, at first I thought this was a good idea, but now I wonder: what is the application supposed to do if the web process becomes unresponsive? Epiphany would want to kill the web process and load an error page, but our API provides no way to do that. Ideally webkit_web_view_load_uri() (or webkit_web_view_load_alternate_html()) would suffice, but in practice it doesn't work: when the web process is unresponsive, this API will hang too. Test here: https://www.w3schools.com/html/tryit.asp?filename=tryhtml5_video. Replace the HTML content with <script>while (1);</script> and then try navigating to a new page using Epiphany's address bar or however you trigger a webkit_web_view_load_uri() in your application. It will hang. To avoid this currently, we have to destroy and recreate the WebKitWebView in order to get a new process in which to display the error page, which is disruptive. So I wonder, what is your application planning to do with this? Maybe our API layer could handle killing the web process itself if it detects it is unresponsive, without exposing new public API? Somehow, we should try to make recovering the current WebKitWebView possible.
Comment on attachment 425876 [details] Patch Patch LGTM, with a couple of nits to take into account before landing :-) View in context: https://bugs.webkit.org/attachment.cgi?id=425876&action=review > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:1341 > + This added empty line is not really needed. > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:4737 > + g_object_notify(G_OBJECT(webView), "is-web-process-responsive"); I see that you have rebased on top of the changes from bug #224366 (nice!), but I think you might have missed changing this property notification to use _notify_by_pspec(): g_object_notify_by_pspec(G_OBJECT(webView), sObjProperties[PROP_IS_WEB_PROCESS_RESPONSIVE]); Please change this before landing.
Created attachment 425881 [details] Patch
(In reply to Michael Catanzaro from comment #9) > Hm, at first I thought this was a good idea, but now I wonder: what is the > application supposed to do if the web process becomes unresponsive? Epiphany > would want to kill the web process and load an error page, but our API > provides no way to do that. Ideally webkit_web_view_load_uri() (or > webkit_web_view_load_alternate_html()) would suffice, but in practice it > doesn't work: when the web process is unresponsive, this API will hang too. > Test here: https://www.w3schools.com/html/tryit.asp?filename=tryhtml5_video. > Replace the HTML content with <script>while (1);</script> and then try > navigating to a new page using Epiphany's address bar or however you trigger > a webkit_web_view_load_uri() in your application. It will hang. To avoid > this currently, we have to destroy and recreate the WebKitWebView in order > to get a new process in which to display the error page, which is disruptive. > > So I wonder, what is your application planning to do with this? Exactly as you mentioned, destroying the WebKitWebView and create it again. But I'm targeting a very specific case. > Maybe our API layer could handle killing the web process itself if it > detects it is unresponsive, without exposing new public API? Somehow, we > should try to make recovering the current WebKitWebView possible. I think unresponsive doesn't mean irrecoverable. A heavy or badly programmed js script may lock the main thread and make the web process unresponsive. In that case we should probably ask the users whether they want to wait or kill the script. And if the user decides to kill it, then kill the web process. But I guess we need some new API for the apps to be able to do that.
We probably don't need new API... fixing the existing API to not hang would suffice. Anyway if your plan is to destroy and recreate the web view, then I suppose this is good, even if that's too heavy-handed for GTK apps to do.
(In reply to Michael Catanzaro from comment #13) > We probably don't need new API... fixing the existing API to not hang would > suffice. I mean: we probably don't need new API beyond the API you've already added in this patch. (In theory, webkit_web_view_load_uri() should trigger a process swap if the current web process is unresponsive. I wonder where it currently gets stuck.)
Thanks Michael for bringing in the discussion about what an application can do if a web view is unresponsive. I think for now we can land this one patch, but it would be actually a very good thing to write a few lines in the documentation of the property indicating what can be be *currently* done to recover from an unresponsive view, and also some words explaining that a web view that has become unresponsive may be responsive again after a while. How does this sound?
I think it's good point, we should investigate why load a new uri or reload doesn't work, that's how we currently recover from crashes.
(In reply to Carlos Garcia Campos from comment #16) > I think it's good point, we should investigate why load a new uri or reload > doesn't work, that's how we currently recover from crashes. Ok, I'll land this and open a new bug to check those cases. My bet is that we're not checking whether the web process is responsive or not when trying to load a new page (through reload, load new uri or load alernate)
Committed r275936 (236498@main): <https://commits.webkit.org/236498@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 425881 [details].
(In reply to Miguel Gomez from comment #17) > Ok, I'll land this and open a new bug to check those cases. My bet is that > we're not checking whether the web process is responsive or not when trying > to load a new page (through reload, load new uri or load alernate) Bug #224533