Bug 220363 – [WPE][GTK] Ensure URI scheme handler is not registered multiple times

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

[Michael Catanzaro]

Currently we have a debug ASSERT to ensure a URI scheme handler is not registered more than once, but nothing checks this in release builds.

See: https://gitlab.gnome.org/GNOME/evolution/-/issues/1286

[Milan Crha]

Be careful with the outcome, considering what it can do with older applications, where you update WebKitGTK at (without updating the affected applications), please.

[Michael Catanzaro]

Created attachment 417094 [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]

(In reply to Milan Crha from comment #1)
> Be careful with the outcome, considering what it can do with older
> applications, where you update WebKitGTK at (without updating the affected
> applications), please.

Thing is, there's no question what the correct behavior is: emit a critical and return early. Duplicate registrations cannot possibly work as expected and should be ignored.

I guess with Evolution, the behavior you were actually getting was for the second registration to take precedence over the previous registration? If so, we're just going to have to update Evolution in tandem with WebKit, or tell distros to patch Evolution downstream. That's OK as long as we don't forget to tell distros to handle this with care.

[Michael Catanzaro]

Ping reviewers

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #5)
> Ping reviewers

[Carlos Garcia Campos]

Comment on attachment 417094 [details]
Patch

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

> Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:1242
> +    if (context->priv->uriSchemeHandlers.get(scheme)) {
> +        g_critical("Cannot register URI scheme %s more than once", scheme);
> +        return;
> +    }

I guess a g_warning should be enough, since this is not fatal in the end. Instead of this early return, we could change the "set" below by an "add" and check if addResult.isNewEntry is false.

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #7)
> > Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:1242
> > +    if (context->priv->uriSchemeHandlers.get(scheme)) {
> > +        g_critical("Cannot register URI scheme %s more than once", scheme);
> > +        return;
> > +    }
> 
> I guess a g_warning should be enough, since this is not fatal in the end.

g_critical() seems like a better choice because this is undefined behavior (programmer error).

> Instead of this early return, we could change the "set" below by an "add"
> and check if addResult.isNewEntry is false.

Sure.

[Michael Catanzaro]

Created attachment 417878 [details]
Patch

[EWS]

commit-queue failed to commit attachment 417878 [details] to WebKit repository.

[Michael Catanzaro]

commit-queue error is:

ERROR from SVN:
Item is out of date: File '/trunk/Source/WebCore/ChangeLog' is out of date
W: 4e3e4e0e2a88e15386956b2a49eafbeb5ac8e2cc and refs/remotes/origin/main differ, using rebase:
:040000 040000 41d277ce2779148216363a2d1801423625051a8a 881cae33229fc437cba1cdd4a5f7318c45ca5f24 M	Source
Current branch main is up to date.
ERROR: Not all changes have been committed into SVN, however the committed
ones (if any) seem to be successfully integrated into the working tree.
Please see the above messages for details.

Not sure what's going on here. Let me rebase and try again....

[Michael Catanzaro]

P.S. Let's avoid backporting this commit.

[Michael Catanzaro]

Created attachment 417967 [details]
Patch for landing

[EWS]

Committed r271647: <https://trac.webkit.org/changeset/271647>

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