Bug 230556 – REGRESSION(241918@main): [WPE][GTK] New test is timing out on bots

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

[Michael Catanzaro]

Unfortunately the new test added in 241918@main is timing out on the bots. :/

It works perfectly fine for me locally, so I cannot do much about this. If any other GTK/WPE maintainers are able to reproduce, or spot anything clearly wrong with the test, that would be interesting to know. Otherwise, my suggestion is to just remove this new test. I considered marking it expected fail, but I think it's more valuable to keep the original test passing, so instead I will revert the changes to this test.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #0)
> Unfortunately the new test added in 241918@main is timing out on the bots. :/
> 
> It works perfectly fine for me locally, so I cannot do much about this.

Ah wait, this is wrong. I got confused:

 * In the last version of *my* patch from https://bugs.webkit.org/show_bug.cgi?id=229116#c26, the test was timing out on the EWS, but worked for me locally. 
 * But in the patch that Alex landed, it cannot possibly work because WebKitWebContext is still using only the WebURLSchemeTask identifier in its map. That's not enough, because the task identifier is not unique in the web process.

IMO we should change the WebURLSchemeTask::identifier to return std::pair<WebCore::ResourceLoaderIdentifier, WebPageProxyID>, since that is now the *real* identifier type that the UI process must use everywhere.

After making this change, it's possible the test will still fail on the bots. We'll just have to find out.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #1)
> That's not enough, because the task identifier is not unique in the web
> process.

I keep typoing this. The task identifier (ResourceLoaderIdentifier) is not unique in the *UI* process. Alex fixed this in WebURLSchemeHandler by switching to use std::pair<WebCore::ResourceLoaderIdentifier, WebPageProxyIdentifier> to identify tasks, but I think it would be better to return that pair directly from WebURLSchemeTask::identifier. That would fix this everywhere while ensuring that the result of WebURLSchemeTask::identifier actually uniquely identifies the WebURLSchemeTask.

We could do this using a new ObjectIdentifier as well, which is what my original patches did, but then that complicates the implementation of WebURLSchemeHandler, which is why Youenn suggested std::pair<WebCore::ResourceLoaderIdentifier, WebPageProxyIdentifier>.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #2)
> We could do this using a new ObjectIdentifier as well, which is what my
> original patches did, but then that complicates the implementation of
> WebURLSchemeHandler, which is why Youenn suggested
> std::pair<WebCore::ResourceLoaderIdentifier, WebPageProxyIdentifier>.

Well I think this will work, but it's a little weird for ::identifier to return a std::pair containing two different ObjectIdentifiers, rather than just returning one ObjectIdentifier.

Alternatively, we could give WebURLSchemeTask its own ObjectIdentifier, and WebURLSchemeHandler can use std::pair<WebURLSchemeTaskIdentifier, WebPageProxyIdentifier> just for convenience to avoid needing to add an extra HashMap.

I'll most likely implement this both ways and then decide which version I prefer. If owners have a strong preference one way or the other, please let me know.

[Michael Catanzaro]

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

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

> Source/WebKit/ChangeLog:11
> +        WebURLSchemeTasks to get messed up. We can fix this by going one step further and changing
> +        it to return a std::pair<WebCore::ResourceLoaderIdentifier, WebPageProxyIdentifier>, which

This fixes the bug for me locally. I want to see if it works for EWS, too.

But it's a little weird, and I'm not sure whether I like it or not, so I haven't set r? yet. I'll try an alternate version where WebURLSchemeTask just uses its own ObjectIdentifier instead of a std::pair next.

[Michael Catanzaro]

There's a third, more minimal option: rename WebURLSchemeTask::identifier to WebURLSchemeTask::resourceLoaderIdentifier, to break the expectation that WebURLSchemeTask::identifier can be used to identify a WebURLSchemeTask. Then just update WebKitWebContext to use std::pair<WebCore::ResourceLoaderIdentifier, WebPageProxyIdentifier> like WebURLSchemeHandler already does since.

[Michael Catanzaro]

Created attachment 438856 [details]
Patch

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #7)
> There's a third, more minimal option: rename WebURLSchemeTask::identifier to
> WebURLSchemeTask::resourceLoaderIdentifier, to break the expectation that
> WebURLSchemeTask::identifier can be used to identify a WebURLSchemeTask.
> Then just update WebKitWebContext to use
> std::pair<WebCore::ResourceLoaderIdentifier, WebPageProxyIdentifier> like
> WebURLSchemeHandler already does since.

Alex, I think you'll like this approach since it doesn't change the cross-platform code except for renaming WebURLSchemeTask::identifier -> WebURLSchemeTask::resourceLoaderID. OK?

[Alex Christensen]

looks great

[Michael Catanzaro]

Thanks.

[EWS]

Committed r282843 (241974@main): <https://commits.webkit.org/241974@main>

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

[Michael Catanzaro]

Looks like this test is still timing out on the WPE release bot. But the GTK release bot is happy. The stderr on the bot has a bunch of errors coming from wpebackend-fdo:

WPE-FDO-WARNING **: 15:39:18.371: Failed to send message 67 to socket: Error sending data: Broken pipe

My guess is WPE port just cannot handle 50 web views at once. When I tried 100 web views with the GTK port, it died due to errors in wpebackend-fdo, so could be something similar. Dunno.

At least I think the URI scheme handler bug is fixed.