Bug 216123 – [GTK][WPE] WEBKIT_PLUGIN_ERROR_WILL_HANDLE_LOAD returned when plugins are disabled

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Michael Catanzaro]

To reproduce, check out this Epiphany commit:

https://gitlab.gnome.org/GNOME/epiphany/-/commit/868de06fca4018bb5eaf61bed8dd8c952e813ec2

Then try to play this video:

https://gnome.modular.im/_matrix/media/r0/download/gnome.modular.im/c7fc680ae099cbd40063a50c81c944a9e26c4922/Peek

Using WebKitGTK 2.29.91, the load will fail with WEBKIT_PLUGIN_ERROR_WILL_HANDLE_LOAD. But Epiphany unconditionally sets enable-plugins to FALSE, so this should be impossible. It fails the same way with WebKit trunk, where WebKitPluginError is deprecated and plugin support has been totally removed.

[Carlos Garcia Campos]

So, the problem here is that pluginWillHandleLoadError has nothing to do with plugins, at least with NPAPI plugins. The error happens when the document is a media document, and the load will be handled by the media engine.

[Carlos Garcia Campos]

I'm not sure how to fix this in a backwards compatibility way. Good thing is that error codes are unique even for different domains, so we could add a new error to the policy domain, for example, using the same error code as pluginWillHandleLoadError. Applications just checking the error code would still work, but not the ones also checking the domain.

[Carlos Garcia Campos]

A quick search in debian shows that only cog and epiphany check the domain. Epiphany would still work as long as the new error is added to network or policy domains, and cog would break because it uses g_error_matches().

[Carlos Garcia Campos]

We can also un-deprecate the plugin error domain and only deprecate the individual error codes, except the WEBKIT_PLUGIN_ERROR_WILL_HANDLE_LOAD and document this is about media engine handle the load. It would still be confusing, but backwards compatible.

[Michael Catanzaro]

Do we really need to return any error at all? It looks like what happens is:

1. We emit load-failed with WEBKIT_PLUGIN_ERROR_WILL_HANDLE_LOAD
2. Epiphany ignores the error.
3. WebKit loads the content properly.

The error seems superfluous, right? Does it really need to be exposed to the browser? Why does load-failed happen at all? Seems like we could skip that step?

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #5)
> Do we really need to return any error at all? It looks like what happens is:
> 
> 1. We emit load-failed with WEBKIT_PLUGIN_ERROR_WILL_HANDLE_LOAD
> 2. Epiphany ignores the error.
> 3. WebKit loads the content properly.
> 
> The error seems superfluous, right? Does it really need to be exposed to the
> browser? Why does load-failed happen at all? Seems like we could skip that
> step?

I also thought about that, I don't think any app really want to handle the error, but the same applies to WEBKIT_POLICY_ERROR_FRAME_LOAD_INTERRUPTED_BY_POLICY_CHANGE and WEBKIT_NETWORK_ERROR_CANCELLED, I guess, but ephy is handling WEBKIT_POLICY_ERROR_FRAME_LOAD_INTERRUPTED_BY_POLICY_CHANGE to close the tab when there's just a download, I think. The alternative is to not emit load-failed, but the load didn't really succeed either.

[Michael Catanzaro]

I added debug. What happens with WebKit trunk is:

** (epiphany:26619): WARNING **: 08:54:24.839: LOAD STARTED: https://gnome.modular.im/_matrix/media/r0/download/gnome.modular.im/c7fc680ae099cbd40063a50c81c944a9e26c4922/Peek

** (epiphany:26619): WARNING **: 08:54:25.524: LOAD COMMITTED: https://gnome.modular.im/_matrix/media/r0/download/gnome.modular.im/c7fc680ae099cbd40063a50c81c944a9e26c4922/Peek

** (epiphany:26619): WARNING **: 08:54:26.577: LOAD FAILED: https://gnome.modular.im/_matrix/media/r0/download/gnome.modular.im/c7fc680ae099cbd40063a50c81c944a9e26c4922/Peek

** (epiphany:26619): WARNING **: 08:54:26.578: LOAD FINISHED: https://gnome.modular.im/_matrix/media/r0/download/gnome.modular.im/c7fc680ae099cbd40063a50c81c944a9e26c4922/Peek

** (epiphany:26619): WARNING **: 08:54:26.582: LOAD STARTED: https://gnome.modular.im/_matrix/media/r0/download/gnome.modular.im/c7fc680ae099cbd40063a50c81c944a9e26c4922/Peek

** (epiphany:26619): WARNING **: 08:54:26.591: LOAD COMMITTED: https://gnome.modular.im/_matrix/media/r0/download/gnome.modular.im/c7fc680ae099cbd40063a50c81c944a9e26c4922/Peek

** (epiphany:26619): WARNING **: 08:54:26.592: LOAD FINISHED: https://gnome.modular.im/_matrix/media/r0/download/gnome.modular.im/c7fc680ae099cbd40063a50c81c944a9e26c4922/Peek

So: we start load, fail load, finish load, then do it all over. With WebKitGTK 2.29.92, WebKit displays the media controls, but the media controls are broken and unable to actually play the media due to bug #183259. That's the behavior we want to get back to. But with WebKitGTK trunk, Epiphany displays an error: "Plug-in handled load." That's a WebKit error page, though, not the fancy Epiphany error page. If I check out Epiphany commit https://gitlab.gnome.org/GNOME/epiphany/-/commit/868de06fca4018bb5eaf61bed8dd8c952e813ec2 then I get the same with a fancy Epiphany error page. So just changing the error domain will not be enough to fix this bug, because something else is wrong inside WebKit. I tried this:

diff --git a/Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp b/Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp
index 191007b6bb66..bb6c1618fbe5 100644
--- a/Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp
+++ b/Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp
@@ -470,7 +470,8 @@ void WebKitWebViewClient::didReceiveUserMessage(WKWPE::View&, UserMessage&& mess
 static gboolean webkitWebViewLoadFail(WebKitWebView* webView, WebKitLoadEvent, const char* failingURI, GError* error)
 {
     if (g_error_matches(error, WEBKIT_NETWORK_ERROR, WEBKIT_NETWORK_ERROR_CANCELLED)
-        || g_error_matches(error, WEBKIT_POLICY_ERROR, WEBKIT_POLICY_ERROR_FRAME_LOAD_INTERRUPTED_BY_POLICY_CHANGE))
+        || g_error_matches(error, WEBKIT_POLICY_ERROR, WEBKIT_POLICY_ERROR_FRAME_LOAD_INTERRUPTED_BY_POLICY_CHANGE)
+        || g_error_matches(error, WEBKIT_POLICY_ERROR, WEBKIT_PLUGIN_ERROR_WILL_HANDLE_LOAD))
         return FALSE;
 
     GUniquePtr<char> htmlString(g_strdup_printf("<html><body>%s</body></html>", error->message));

But that wasn't enough to make it work.

I think desired behavior would be load started -> load committed -> load finished, once, without any errors. But on the other hand, I'm also aware that attempting to paper over internal WebKit loader behavior at the API level has caused very tricky bugs in the past, so it's probably risky to try changing that.

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #7)
> I added debug. What happens with WebKit trunk is:
> 
> ** (epiphany:26619): WARNING **: 08:54:24.839: LOAD STARTED:
> https://gnome.modular.im/_matrix/media/r0/download/gnome.modular.im/
> c7fc680ae099cbd40063a50c81c944a9e26c4922/Peek
> 
> ** (epiphany:26619): WARNING **: 08:54:25.524: LOAD COMMITTED:
> https://gnome.modular.im/_matrix/media/r0/download/gnome.modular.im/
> c7fc680ae099cbd40063a50c81c944a9e26c4922/Peek
> 
> ** (epiphany:26619): WARNING **: 08:54:26.577: LOAD FAILED:
> https://gnome.modular.im/_matrix/media/r0/download/gnome.modular.im/
> c7fc680ae099cbd40063a50c81c944a9e26c4922/Peek
> 
> ** (epiphany:26619): WARNING **: 08:54:26.578: LOAD FINISHED:
> https://gnome.modular.im/_matrix/media/r0/download/gnome.modular.im/
> c7fc680ae099cbd40063a50c81c944a9e26c4922/Peek
> 
> ** (epiphany:26619): WARNING **: 08:54:26.582: LOAD STARTED:
> https://gnome.modular.im/_matrix/media/r0/download/gnome.modular.im/
> c7fc680ae099cbd40063a50c81c944a9e26c4922/Peek
> 
> ** (epiphany:26619): WARNING **: 08:54:26.591: LOAD COMMITTED:
> https://gnome.modular.im/_matrix/media/r0/download/gnome.modular.im/
> c7fc680ae099cbd40063a50c81c944a9e26c4922/Peek
> 
> ** (epiphany:26619): WARNING **: 08:54:26.592: LOAD FINISHED:
> https://gnome.modular.im/_matrix/media/r0/download/gnome.modular.im/
> c7fc680ae099cbd40063a50c81c944a9e26c4922/Peek
> 
> So: we start load, fail load, finish load, then do it all over. With
> WebKitGTK 2.29.92, WebKit displays the media controls, but the media
> controls are broken and unable to actually play the media due to bug
> #183259. That's the behavior we want to get back to. But with WebKitGTK
> trunk, Epiphany displays an error: "Plug-in handled load."

Yes, because you stopped handling that error in ephy.

> That's a WebKit
> error page, though, not the fancy Epiphany error page.

Yes, because you stopped handling that error in ephy, the default handler is called and it shows the default error page, because WebKit also stopped handling that error in the default handler.

> If I check out
> Epiphany commit
> https://gitlab.gnome.org/GNOME/epiphany/-/commit/
> 868de06fca4018bb5eaf61bed8dd8c952e813ec2 then I get the same with a fancy
> Epiphany error page. So just changing the error domain will not be enough to
> fix this bug, because something else is wrong inside WebKit. I tried this:

I meant adding a new error with the same code, but different domain, that needs to be handled by ephy. Ephy returns early if error domain is not network or policy, then it handles errors individually. If we add the new error to network or policy and ephy keeps current code just adding the new error it should just work.

> diff --git a/Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp
> b/Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp
> index 191007b6bb66..bb6c1618fbe5 100644
> --- a/Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp
> +++ b/Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp
> @@ -470,7 +470,8 @@ void
> WebKitWebViewClient::didReceiveUserMessage(WKWPE::View&, UserMessage&& mess
>  static gboolean webkitWebViewLoadFail(WebKitWebView* webView,
> WebKitLoadEvent, const char* failingURI, GError* error)
>  {
>      if (g_error_matches(error, WEBKIT_NETWORK_ERROR,
> WEBKIT_NETWORK_ERROR_CANCELLED)
> -        || g_error_matches(error, WEBKIT_POLICY_ERROR,
> WEBKIT_POLICY_ERROR_FRAME_LOAD_INTERRUPTED_BY_POLICY_CHANGE))
> +        || g_error_matches(error, WEBKIT_POLICY_ERROR,
> WEBKIT_POLICY_ERROR_FRAME_LOAD_INTERRUPTED_BY_POLICY_CHANGE)
> +        || g_error_matches(error, WEBKIT_POLICY_ERROR,
> WEBKIT_PLUGIN_ERROR_WILL_HANDLE_LOAD))
>          return FALSE;
>  
>      GUniquePtr<char>
> htmlString(g_strdup_printf("<html><body>%s</body></html>", error->message));
> 
> But that wasn't enough to make it work.

Because the patch is wrong, you are using WEBKIT_POLICY_ERROR domains for the WEBKIT_PLUGIN_ERROR_WILL_HANDLE_LOAD error.

> I think desired behavior would be load started -> load committed -> load
> finished, once, without any errors. But on the other hand, I'm also aware
> that attempting to paper over internal WebKit loader behavior at the API
> level has caused very tricky bugs in the past, so it's probably risky to try
> changing that.

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #8)
> I meant adding a new error with the same code, but different domain, that
> needs to be handled by ephy. Ephy returns early if error domain is not
> network or policy, then it handles errors individually. If we add the new
> error to network or policy and ephy keeps current code just adding the new
> error it should just work.

It's not enough. I tested this with and without the Ephy patch and it was broken with WebKit trunk either way. (But 2.29.92 is fine, without the Ephy patch.)
 
> Because the patch is wrong, you are using WEBKIT_POLICY_ERROR domains for
> the WEBKIT_PLUGIN_ERROR_WILL_HANDLE_LOAD error.

Ah, oops. :P That explains it. So with that portion of r265753 reverted *properly*, then it works again. (Well, it "works" ignoring bug #183259.) So we just need to remember to change that as well if we change the error type.

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #9)
> (In reply to Carlos Garcia Campos from comment #8)
> > I meant adding a new error with the same code, but different domain, that
> > needs to be handled by ephy. Ephy returns early if error domain is not
> > network or policy, then it handles errors individually. If we add the new
> > error to network or policy and ephy keeps current code just adding the new
> > error it should just work.
> 
> It's not enough. I tested this with and without the Ephy patch and it was
> broken with WebKit trunk either way. (But 2.29.92 is fine, without the Ephy
> patch.)

Because we haven't added the new error that ephy should be handling instead. Anyway, we should handle the error in webkit default handler too, in that case, current ephy code should work, since it returns FALSE to let the default handler run.
 
> > Because the patch is wrong, you are using WEBKIT_POLICY_ERROR domains for
> > the WEBKIT_PLUGIN_ERROR_WILL_HANDLE_LOAD error.
> 
> Ah, oops. :P That explains it. So with that portion of r265753 reverted
> *properly*, then it works again. (Well, it "works" ignoring bug #183259.) So
> we just need to remember to change that as well if we change the error type.

Yes, it was a mistake to stop handling the error in the default handler, because I also assumed it was a NPAPI plugin error, so I'll bring it back.

[Michael Catanzaro]

Hm, we still need to do something here:

[132/276] Compiling C object embed/libephyembed.a.p/ephy-web-view.c.o
../../../../Projects/epiphany/embed/ephy-web-view.c: In function ‘load_failed_cb’:
../../../../Projects/epiphany/embed/ephy-web-view.c:2297:7: warning: ‘webkit_plugin_error_quark’ is deprecated [-Wdeprecated-declarations]
 2297 |       error->domain != WEBKIT_PLUGIN_ERROR) {
      |       ^~~~~
In file included from /home/mcatanzaro/Projects/GNOME/install/include/webkitgtk-4.0/webkit2/webkit2.h:46,
                 from ../../../../Projects/epiphany/embed/ephy-web-view.h:24,
                 from ../../../../Projects/epiphany/embed/ephy-web-view.c:23:
/home/mcatanzaro/Projects/GNOME/install/include/webkitgtk-4.0/webkit2/WebKitError.h:164:1: note: declared here
  164 | webkit_plugin_error_quark              (void);
      | ^~~~~~~~~~~~~~~~~~~~~~~~~


But we cannot remove this code due to this bug. Easiest solution is to un-deprecate the API.

[Carlos Garcia Campos]

Created attachment 413410 [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 413410 [details]
Patch

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

LGTM

> Source/WebKit/UIProcess/API/gtk/WebKitError.h:84
> + * @WEBKIT_PLUGIN_ERROR_WILL_HANDLE_LOAD: Load failure since media player handles the load.

Perhaps: "Preliminary load failure for media content types. A new load will be started to perform the media load." Or something like that. It's pretty confusing that the initial load failure gets exposed at the API level, but papering over that seems dangerous, so oh well.

[Carlos Garcia Campos]

Committed r269618: <https://trac.webkit.org/changeset/269618>