Bug 224359 – [GTK][WPE] Add a property to the WebKitWebView indicating whether the web process is responsive

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Miguel Gomez]

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.

[Miguel Gomez]

Created attachment 425592 [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

[Carlos Garcia Campos]

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

[Adrian Perez]

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.

[Carlos Garcia Campos]

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?

[Miguel Gomez]

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

[Adrian Perez]

(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 🤷️

[Miguel Gomez]

Created attachment 425876 [details]
Patch

[Michael Catanzaro]

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.

[Adrian Perez]

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.

[Miguel Gomez]

Created attachment 425881 [details]
Patch

[Miguel Gomez]

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

[Michael Catanzaro]

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.

[Michael Catanzaro]

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

[Adrian Perez]

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?

[Carlos Garcia Campos]

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.

[Miguel Gomez]

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

[EWS]

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

[Michael Catanzaro]

(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