Bug 220363 - [WPE][GTK] Ensure URI scheme handler is not registered multiple times
Summary: [WPE][GTK] Ensure URI scheme handler is not registered multiple times
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: PC Linux
: P2 Normal
Assignee: Michael Catanzaro
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-01-06 07:30 PST by Michael Catanzaro
Modified: 2021-01-20 07:59 PST (History)
7 users (show)

See Also:


Attachments
Patch (1.48 KB, patch)
2021-01-06 09:26 PST, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (1.92 KB, patch)
2021-01-19 08:28 PST, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch for landing (1.95 KB, patch)
2021-01-20 07:22 PST, Michael Catanzaro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Catanzaro 2021-01-06 07:30:21 PST
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
Comment 1 Milan Crha 2021-01-06 09:09:09 PST
Be careful with the outcome, considering what it can do with older applications, where you update WebKitGTK at (without updating the affected applications), please.
Comment 2 Michael Catanzaro 2021-01-06 09:26:42 PST
Created attachment 417094 [details]
Patch
Comment 3 EWS Watchlist 2021-01-06 09:27:32 PST
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 4 Michael Catanzaro 2021-01-06 09:33:50 PST
(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.
Comment 5 Michael Catanzaro 2021-01-11 11:34:09 PST
Ping reviewers
Comment 6 Michael Catanzaro 2021-01-18 09:03:23 PST
(In reply to Michael Catanzaro from comment #5)
> Ping reviewers
Comment 7 Carlos Garcia Campos 2021-01-19 00:22:57 PST
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.
Comment 8 Michael Catanzaro 2021-01-19 08:11:36 PST
(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.
Comment 9 Michael Catanzaro 2021-01-19 08:28:24 PST
Created attachment 417878 [details]
Patch
Comment 10 EWS 2021-01-20 00:43:03 PST
commit-queue failed to commit attachment 417878 [details] to WebKit repository.
Comment 11 Michael Catanzaro 2021-01-20 07:20:40 PST
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....
Comment 12 Michael Catanzaro 2021-01-20 07:21:48 PST
P.S. Let's avoid backporting this commit.
Comment 13 Michael Catanzaro 2021-01-20 07:22:00 PST
Created attachment 417967 [details]
Patch for landing
Comment 14 EWS 2021-01-20 07:59:47 PST
Committed r271647: <https://trac.webkit.org/changeset/271647>

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