Bug 229116 – WebKit might load custom URI scheme request content multiple times

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Yu-Wei Wu]

Here's the repo and steps to reproduce the subtle bug I encountered:
https://github.com/wusyong/gtkbrowser

It opens 10 webview windows. All of them share the same web context which registers a "gtk" URL scheme that loads a simple HTML.
But some window's pages will be stolen by others. (One window will show nothing and the other will display two HTML files)
I've tried adding locks but it has to be added after "load_changed" signal. Otherwise, the situation is still the same. And if I do so, it'll take a long time to show all windows (each window is displayed one by one)

[Michael Catanzaro]

Unfortunately WebKit is invoking the WebKitURISchemeRequestCallback multiple times with the same WebKitURISchemeRequest object. Then WebKit gets very confused because the application called webkit_uri_scheme_request_finish() multiple times with the same object, but this is WebKit's fault for reusing the same WebKitURISchemeRequest in the first place. It actually trips an assert in WebKit::WebKitURISchemeHandler::platformStartTask, in WebKitWebContext.cpp:

ASSERT(addResult.isNewEntry);

but we compile out ASSERTs in release builds, and nobody tests debug builds because they ASSERT too much, because nobody tests debug builds. :)

The problem is that the UI process uses task IDs to distinguish between different WebURLSchemeTask objects, and reasonably expects the task IDs to be globally-unique. But the task IDs are actually generated in the web process in WebURLSchemeTaskProxy::startLoading using ResourceLoader::identifier, which uses ProgressTracker::createUniqueIdentifier, which just increments a counter variable, so they're only unique to a given web process, not unique in the web process. In your test program, all 10 tasks have the same ID: 1. Oops!

It's a cross-platform bug btw, nothing GTK-specific here. Probably not hard to fix, but I'm too tired to think it through tonight.

[Michael Catanzaro]

Created attachment 437584 [details]
Patch

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #1)
> It's a cross-platform bug btw, nothing GTK-specific here. Probably not hard
> to fix, but I'm too tired to think it through tonight.

Well actually it was easy. The attached patch fixes the bug, but I need to write an API test before it is ready for real review. That said, I'm CCing WK2 owners early to make sure owners are OK with this approach. Basically I just have WebURLSchemeTask store the task ID it receives from the web process and use it for all communications with the web process, but expose an entirely different task ID to the UI process.

BTW: great bug report, your test program made it easy to reproduce.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #1)
> so they're only unique to a given web process, not unique in the
> web process.

I meant to write: not unique in the UI process.

[Michael Catanzaro]

Created attachment 437779 [details]
Test case

For posterity, I'm attaching the test case from https://github.com/wusyong/gtkbrowser/blob/master/browser.c

[Michael Catanzaro]

Created attachment 437816 [details]
Patch

[Michael Catanzaro]

Hi owners, this patch needs owner review. Thank you!

[Darin Adler]

Comment on attachment 437816 [details]
Patch

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

> Source/WebKit/ChangeLog:13
> +        unique in the web process. Ouch! Since we have multiple tasks with the same ID, they clobber

s/in the web process/in the UI process/

[Michael Catanzaro]

Created attachment 437817 [details]
Patch

[Michael Catanzaro]

(In reply to Darin Adler from comment #8) 
> s/in the web process/in the UI process/

Oops, yes, copied that typo straight from comment #1.

[Michael Catanzaro]

Created attachment 437818 [details]
Patch

[Michael Catanzaro]

Created attachment 437820 [details]
Patch

[Michael Catanzaro]

Created attachment 437825 [details]
Patch

[Alex Christensen]

Comment on attachment 437825 [details]
Patch

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

I looked into whether this problem affects WKWebView and it doesn't seem that it does because in WKWebView._initializeWithConfiguration we create a WebURLSchemeHandlerCocoa for each WKWebView and for each scheme.  You might consider doing something similar in webkit_web_context_register_uri_scheme to fix this instead to make the ports more similar instead of adding code only needed by one port in common port-agnostic code.

> Source/WebKit/UIProcess/WebURLSchemeTask.h:105
>      uint64_t m_identifier;
> +    uint64_t m_webProcessTaskIdentifier;

We want to use strongly typed identifiers instead of expanding the use of uint64_t.  See PageIdentifier.h et al.

[Michael Catanzaro]

(In reply to Alex Christensen from comment #14)
> I looked into whether this problem affects WKWebView and it doesn't seem
> that it does because in WKWebView._initializeWithConfiguration we create a
> WebURLSchemeHandlerCocoa for each WKWebView and for each scheme.  You might
> consider doing something similar in webkit_web_context_register_uri_scheme
> to fix this instead to make the ports more similar instead of adding code
> only needed by one port in common port-agnostic code.

Hm, that seems OK to me, but it would only make sense if we make some bigger cross-platform changes in WebURLSchemeHandler.h. Currently WebURLSchemeHandler::startTask and WebURLSchemeHandler::stopTask take a WebPageProxy parameter, implying that each task may safely use a different WebPageProxy. There is also WebURLSchemeHandler::stopAllTasksForPage. If we go with your approach, then we should change WebURLSchemeHandler to take the WebPageProxy as a parameter of its constructor instead, drop it as a parameter from the other functions, and rename stopAllTasksForPage to just stopAllTasks. Agreed? I kinda prefer my existing solution, but I'll try to implement this if you prefer. But I don't want to leave WebURLSchemeHandler broken, so it requires cross-platform changes either way.

> > Source/WebKit/UIProcess/WebURLSchemeTask.h:105
> >      uint64_t m_identifier;
> > +    uint64_t m_webProcessTaskIdentifier;
> 
> We want to use strongly typed identifiers instead of expanding the use of
> uint64_t.  See PageIdentifier.h et al.

Yeah OK, makes sense.

[Alex Christensen]

I don't think I have a strong opinion on the approach.  Your approach actually seemed fine to me, and I think it matches the WKURLSchemeHandler API closer to have one object that can be attached to multiple pages.  We may want to change WKWebView._initializeWithConfiguration.

But the strongly typed identifiers will help everything.

[Michael Catanzaro]

OK, in that case I'll change to a strongly-typed identifier but otherwise keep the patch the same.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #17)
> OK, in that case I'll change to a strongly-typed identifier but otherwise
> keep the patch the same.

Actually, changing these to use ObjectIdentifier will require a significant amount of code changes that will significantly complicate this patch. It's cleaner to leave this patch as-is, and convert to ObjectIdentifier in a follow-up patch in a different bug. Will try that now.

[Michael Catanzaro]

(In reply to Alex Christensen from comment #16)
> But the strongly typed identifiers will help everything.

This was a good idea, because it found a bug. My patch actually breaks WebPageProxy::stopURLSchemeTask by misusing the web process identifier in the UI process, which I failed to notice until I tried converting it to strongly typed identifiers. I don't immediately see an easy way to fix it, so will need to continue this next week.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #19)
> This was a good idea, because it found a bug.

FWIW there was actually no bug in the initial patch after all. I just got confused when doing the refactoring to strongly-typed identifiers.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #20)
> FWIW there was actually no bug in the initial patch after all. I just got
> confused when doing the refactoring to strongly-typed identifiers.

Just kidding, there really is a bug, but one level deeper, in WebURLSchemeHandler::stopTask. After my patch, this function receives web process task IDs but expects to operate on the UI process task IDs. Oops.

[Michael Catanzaro]

Created attachment 438183 [details]
Patch

[Michael Catanzaro]

Comment on attachment 438183 [details]
Patch

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

OK Alex, here is round two. This is basically the same as the original patch but with WebURLSchemeHandler::stopTask fixed. OK?

I have a separate patch in bug #230278 that converts to strongly-typed identifiers. That patch is pretty big and would obscure the fix here if combined into one, so I think it's more appropriate to land them separately.

> Source/WebKit/UIProcess/WebURLSchemeHandler.cpp:95
> -void WebURLSchemeHandler::stopTask(WebPageProxy& page, uint64_t taskIdentifier)
> +void WebURLSchemeHandler::stopTaskWithIdentifier(WebPageProxy& page, uint64_t taskIdentifier)

This is a little confusing, but it will be cleaned up in the patch in bug #230278.

[Alex Christensen]

Comment on attachment 438183 [details]
Patch

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

I really think the strong typing needs to be done before this.  It's hard to see whether this is correct or not.

> Source/WebKit/UIProcess/WebURLSchemeHandler.cpp:113
> +    for (auto& t : m_tasks.values()) {
> +        if (t->webProcessTaskID() == webProcessTaskIdentifier) {

This isn't good.  The whole point of a HashMap is so that we don't have to look at all the values.  This takes a O(1) algorithm and makes it an O(n) algorithm.

[Michael Catanzaro]

(In reply to Alex Christensen from comment #24)
> I really think the strong typing needs to be done before this.  It's hard to
> see whether this is correct or not.

Problem is strong typing works really hard to avoid bugs like this, so it's quite awkward to introduce the strong typing without fixing this bug at the same time. It's possible, but I would have to explicitly write the mistake into the strong typing patch by explicitly converting from WebURLSchemeTaskProxyIdentifier to uint64 to WebURLSchemeTaskIdentifier. Then this follow-up patch would be simplified to just removing that forced mistake. So really it doesn't make sense to do it that way. Another alternative would be to do it all in one patch, which is actually what I have in bug #230278 right now, but that really obscures the nature of the fix by hiding it among a much larger diff. So that's why I split it up this way.

ObjectIdentifier is pretty great. The more we use it, the fewer mistakes we'll have.

> > Source/WebKit/UIProcess/WebURLSchemeHandler.cpp:113
> > +    for (auto& t : m_tasks.values()) {
> > +        if (t->webProcessTaskID() == webProcessTaskIdentifier) {
> 
> This isn't good.  The whole point of a HashMap is so that we don't have to
> look at all the values.  This takes a O(1) algorithm and makes it an O(n)
> algorithm.

That's true. I was thinking it'd be OK because I would expect the number of URL scheme tasks to always be very small (in general, there should only be one), but considering it today, I agree with you: there is already a second HashMap here to avoid iterating over the primary map elsewhere in this file, and its presence implies a design decision to avoid the cost of looping, and it would be inconsistent to loop here but not there.

The solution to avoid this is to add a third HashMap to WebURLSchemeHandler. Extra storage cost should be negligible since, again, the normal case is going to be zero or one task in flight. There will be some additional accounting complexity in the other functions of this file since WebURLSchemeHandler would be maintaining three HashMaps in sync rather than the current two.

So there is a trade-off between time vs. storage. But I suspect it doesn't matter much at all either way, because the maps should be very small, so I'm fine with whichever way you prefer. Do you want me to add the third HashMap? (If not, we would need to retain the loop here.)

[Michael Catanzaro]

Hm, it looks like this latest patch actually broke the URI scheme handler test for both GTK and Mac ports (two separate tests). That's no good. I'll need to investigate it.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #26)
> Hm, it looks like this latest patch actually broke the URI scheme handler
> test for both GTK and Mac ports (two separate tests). That's no good. I'll
> need to investigate it.

Unfortunately the GTK test always passes for me locally. Drat. That makes this hard. :/

[Alex Christensen]

Created attachment 438707 [details]
Patch

[EWS]

Committed r282783 (241918@main): <https://commits.webkit.org/241918@main>

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

[Radar WebKit Bug Importer]

<rdar://problem/83324926>

[Michael Catanzaro]

Thanks Alex!

[Yu-Wei Wu]

The comments are detailed and patches are quick.
Realy appreciated. Thank you all!

[Michael Catanzaro]

Unfortunately the new test is timing out on the bots. :/ Follow-up in bug #230556.

[Michael Catanzaro]

(In reply to Yu-Wei Wu from comment #32)
> The comments are detailed and patches are quick.
> Realy appreciated. Thank you all!

Note the version of this patch that landed fixed the cross-platform bug, but the GTK/WPE API is still broken, so doesn't actually help you. We will follow up in that bug #230556.