Bug 230278 – Use ObjectIdentifier for ResourceLoader

• Component: WebKit2
• Status: RESOLVED FIXED

[Michael Catanzaro]

Use ObjectIdentifier for URL scheme handlers and tasks: WebURLSchemeTask, WebURLSchemeTaskProxy, and WebURLSchemeHandler

This patch is not ready quite yet because it currently conflicts with the patch in bug #229116. I'll refresh it after the patch in bug #229116 lands.

[Michael Catanzaro]

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

Created attachment 438178 [details]
Patch

[Michael Catanzaro]

Looks like I have some build errors to fix. It looks like release logs are not enabled for GTK/WPE release builds, because these look like mostly cross-platform build errors that I didn't hit....

[Radar WebKit Bug Importer]

<rdar://problem/83152801>

[Michael Catanzaro]

Created attachment 438282 [details]
Patch

[Alex Christensen]

Comment on attachment 438282 [details]
Patch

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

> Source/WebKit/Shared/URLSchemeTaskParameters.cpp:50
> +    WebURLSchemeHandlerIdentifier handlerIdentifier;

Doesn't Decoder::operator>> work here?

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

This is a loop we don't want to introduce in this patch.

[Michael Catanzaro]

(In reply to Alex Christensen from comment #7)
> Doesn't Decoder::operator>> work here?

I'm not sure. Will find out.

> > Source/WebKit/UIProcess/WebURLSchemeHandler.cpp:107
> > +    for (auto& t : m_tasks.values()) {
> > +        if (t->webProcessTaskID() == webProcessTaskIdentifier) {
> 
> This is a loop we don't want to introduce in this patch.

Yeah it's the same loop we're already discussing in https://bugs.webkit.org/show_bug.cgi?id=229116#c24. Once we agree how to proceed there, I will update this patch to match.

[Alex Christensen]

I think we should make this patch only changing uint64_t to ObjectIdentifier, then we can do another change to address that bug.

[Michael Catanzaro]

(In reply to Alex Christensen from comment #9)
> I think we should make this patch only changing uint64_t to
> ObjectIdentifier, then we can do another change to address that bug.

OK, as you prefer, but note it's going to be a little weird for the reason I mention in https://bugs.webkit.org/show_bug.cgi?id=229116#c25.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #10)
> OK, as you prefer, but note it's going to be a little weird for the reason I
> mention in https://bugs.webkit.org/show_bug.cgi?id=229116#c25.

I decided to avoid that by removing WebURLSchemeTaskIdentifier. Instead, I only introduce WebURLSchemeTaskProxyIdentifier here, that way this patch can misuse it exactly the same as the original code and avoid behavior changes in this bug. Then in bug #229116 I can fix things by introducing WebURLSchemeTaskIndentifier.

[Michael Catanzaro]

(In reply to Alex Christensen from comment #7)
> > Source/WebKit/Shared/URLSchemeTaskParameters.cpp:50
> > +    WebURLSchemeHandlerIdentifier handlerIdentifier;
> 
> Doesn't Decoder::operator>> work here?

It actually does work fine. I must have misread an error message at some point.

[Michael Catanzaro]

Created attachment 438293 [details]
Patch

[Michael Catanzaro]

Comment on attachment 438293 [details]
Patch

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

> Source/WebKit/WebProcess/WebPage/WebURLSchemeTaskProxy.cpp:88
> -    m_urlSchemeHandler.page().send(Messages::WebPageProxy::StartURLSchemeTask(URLSchemeTaskParameters { m_urlSchemeHandler.identifier(), m_coreLoader->identifier(), m_request, m_frame->info() }));
> +    m_urlSchemeHandler.page().send(Messages::WebPageProxy::StartURLSchemeTask(URLSchemeTaskParameters { m_urlSchemeHandler.identifier(), m_identifier, m_request, m_frame->info() }));
>  }
>  
>  void WebURLSchemeTaskProxy::stopLoading()
>  {
>      ASSERT(m_coreLoader);
>      WEBURLSCHEMETASKPROXY_RELEASE_LOG("stopLoading");
> -    m_urlSchemeHandler.page().send(Messages::WebPageProxy::StopURLSchemeTask(m_urlSchemeHandler.identifier(), m_coreLoader->identifier()));
> +    m_urlSchemeHandler.page().send(Messages::WebPageProxy::StopURLSchemeTask(m_urlSchemeHandler.identifier(), m_identifier));

These are the only two changes that aren't a literal translation of the existing code, but it makes no difference because WebCore::ResourceLoader's identifier does not change after it is created. Seems cleaner to avoid converting from that to ObjectIdentifier again when we have a member variable handy already.

[Darin Adler]

Comment on attachment 438293 [details]
Patch

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

> Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:465
> +    if (auto task = m_urlSchemeTasks.take(makeObjectIdentifier<WebURLSchemeTaskProxy>(resourceLoader->identifier()))) {

I don’t understand this recurring pattern. Does the resource loader have an identifier that is a plain old integer; is it a different type of identifier for each different type of resource load? What makes it safe to just assume it’s a scheme task proxy identifier here?

[youenn fablet]

Comment on attachment 438293 [details]
Patch

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

> Source/WebKit/UIProcess/WebURLSchemeTask.cpp:51
> +    // https://bugs.webkit.org/show_bug.cgi?id=229116

s/not not/not/
But also,  m_identifier+m_webPageProxyID is unique within UIProcess.
It could be that paired identifier that could be publicly exposed.

>> Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:465
>> +    if (auto task = m_urlSchemeTasks.take(makeObjectIdentifier<WebURLSchemeTaskProxy>(resourceLoader->identifier()))) {
> 
> I don’t understand this recurring pattern. Does the resource loader have an identifier that is a plain old integer; is it a different type of identifier for each different type of resource load? What makes it safe to just assume it’s a scheme task proxy identifier here?

I also feel like we can do without this change.
Typing ResourceLoader::identifier() is a good thing and we could then reuse ResourceLoaderIdentifier directly.
If needed, there could be a mapping in UIProcess from PageProxyID+ResourceLoaderIdentifier to UIProcess generated WebURLSchemeTaskIdentifier.

> Source/WebKit/WebProcess/Network/WebLoaderStrategy.h:140
>      HashMap<unsigned long, PreconnectCompletionHandler> m_preconnectCompletionHandlers;

All these maps should in the end take the same key type (uint64_t or ResourceLoaderIdentifier).

[Michael Catanzaro]

(In reply to Darin Adler from comment #15)
> > Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:465
> > +    if (auto task = m_urlSchemeTasks.take(makeObjectIdentifier<WebURLSchemeTaskProxy>(resourceLoader->identifier()))) {
> 
> I don’t understand this recurring pattern. Does the resource loader have an
> identifier that is a plain old integer; 

This is correct.

> is it a different type of identifier
> for each different type of resource load?

No, it's just one identifier.

> What makes it safe to just assume
> it’s a scheme task proxy identifier here?

It's safe in the web process because each task corresponds to exactly one resource load. But in the UI process code, it is not safe. That's causing bug #229116 and ultimately that's what I want to fix. But in this intermediate patch, I'm just trying to switch to ObjectIdentifier without changing behavior, since that's the order in which Alex preferred to do this.

That is: it's intentionally incorrect, matching the existing code, as a precursor to fix it in bug #229116.

(In reply to youenn fablet from comment #16)
> s/not not/not/
> But also,  m_identifier+m_webPageProxyID is unique within UIProcess.
> It could be that paired identifier that could be publicly exposed.

Hm, I'm not certain whether that is actually right with PSON. In theory, the WebPageProxy could correspond to multiple different WebPages. Maybe if a URI scheme request for a subresource load that is in flight at the same time that a process swap occurs, there could be confusion? I'm not sure. We should discuss this proposal more in bug #229116.

> I also feel like we can do without this change.
> Typing ResourceLoader::identifier() is a good thing and we could then reuse
> ResourceLoaderIdentifier directly.
> If needed, there could be a mapping in UIProcess from
> PageProxyID+ResourceLoaderIdentifier to UIProcess generated
> WebURLSchemeTaskIdentifier.
> 
> > Source/WebKit/WebProcess/Network/WebLoaderStrategy.h:140
> >      HashMap<unsigned long, PreconnectCompletionHandler> m_preconnectCompletionHandlers;
> 
> All these maps should in the end take the same key type (uint64_t or
> ResourceLoaderIdentifier).

Hm, that does seem elegant, but it would require changes in a lot more code, unrelated to the URI scheme handlers. It's taking me a bit far away from the goal of fixing bug #229116. That said, we should absolutely convert ResourceLoader to use ObjectIdentifier. I would just rather not block bug #229116 on this.

[Alex Christensen]

Created attachment 438397 [details]
Patch

[Michael Catanzaro]

Comment on attachment 438397 [details]
Patch

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

OK, thanks Alex. This obviously turned out better than my approach. I'm surprised you managed such a huge change so quickly.

(Note: I reviewed the first patch you attached.)

> Source/WebCore/loader/DocumentLoader.cpp:2019
> -void DocumentLoader::addSubresourceLoader(ResourceLoader* loader)
> +void DocumentLoader::addSubresourceLoader(ResourceLoader& loader)

Nice pointer -> ref improvements too.

> Source/WebCore/loader/ProgressTracker.cpp:290
>  unsigned long ProgressTracker::createUniqueIdentifier()
>  {
> +    // FIXME: Use a strongly typed identifier for websockets and remove this.
>      return ++s_uniqueIdentifier;
>  }

Ah great, I was going to ask if it was possible to remove this.

> Source/WebCore/loader/WorkerThreadableLoader.cpp:118
> +    // FIXME: m_workerRequestIdentifier should have its own type, distinct from ResourceLoaderIdentifier, which should only be used to identify ResourceLoaders.

Hm, is this FIXME actually practical? You have to eventually cast to ResourceLoaderIdentifier no matter what, right? So you have to ensure the values don't clash with the values of ResourceLoaderIdentifiers. Am I missing something? Unless you have thought of something that I missed, I would just remove the FIXME.

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:1271
> -void NetworkConnectionToWebProcess::prioritizeResourceLoads(Vector<ResourceLoadIdentifier> loadIdentifiers)
> +void NetworkConnectionToWebProcess::prioritizeResourceLoads(const Vector<WebCore::ResourceLoaderIdentifier>& loadIdentifiers)

Oops, good.

> Source/WebKit/UIProcess/ProvisionalPageProxy.h:128
> -    void decidePolicyForResponse(WebCore::FrameIdentifier, FrameInfoData&&, WebCore::PolicyCheckIdentifier, uint64_t navigationID, const WebCore::ResourceResponse&, const WebCore::ResourceRequest&, bool canShowMIMEType, const String& downloadAttribute, bool wasAllowedByInjectedBundle, WebCore::BrowsingContextGroupSwitchDecision, uint64_t listenerID, uint64_t mainResourceLoadIdentifier, const UserData&);
> +    void decidePolicyForResponse(WebCore::FrameIdentifier, FrameInfoData&&, WebCore::PolicyCheckIdentifier, uint64_t navigationID, const WebCore::ResourceResponse&, const WebCore::ResourceRequest&, bool canShowMIMEType, const String& downloadAttribute, bool wasAllowedByInjectedBundle, WebCore::BrowsingContextGroupSwitchDecision, WebCore::ResourceLoaderIdentifier mainResourceLoadIdentifier, uint64_t listenerID, const UserData&);

Wow, I see the parameters here were previously misnamed. Dangerous. Another win for ObjectIdentifier....

> Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:709
> -    loadParameters.identifier = WebLoaderStrategy::generateLoadIdentifier();
> +    loadParameters.identifier =  WebCore::ResourceLoaderIdentifier::generate();

Extra space here.

> Source/WebKit/WebProcess/WebPage/WebURLSchemeHandlerProxy.h:72
> +    // FIXME: This should be a strongly typed identifier.
>      uint64_t m_identifier { 0 };

Do you want to follow up on WebURLSchemeHandlerProxy, or want me to do it? This seems like the least-controversial portion of my patch since it's just a straight 1:1 conversion to use ObjectIdentifier.

[Michael Catanzaro]

Comment on attachment 438397 [details]
Patch

r=me, of course I assume you placate EWS. Looks like there are missing spots in WebFrameLoaderClientIOS.mm. Then I'm not sure what's wrong with the include path on the Windows bot....

[Alex Christensen]

Comment on attachment 438397 [details]
Patch

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

> Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:852
> +    auto mainResourceLoadIdentifier = policyDocumentLoader && policyDocumentLoader->mainResourceLoader() ? policyDocumentLoader->mainResourceLoader()->identifier() : WebCore::ResourceLoaderIdentifier();

This needs to be std::nullopt instead of an empty ResourceLoaderIdentifier because we are sending it across IPC, which only allows valid identifiers.

[Alex Christensen]

Created attachment 438489 [details]
Patch

[Michael Catanzaro]

Careful, you accidentally included a project.pbxproj.orig in the diff.

Also the GTK/WPE EWS is not happy with this version of the patch.

[Michael Catanzaro]

Comment on attachment 438489 [details]
Patch

There's also a Document.cpp.orig... probably more, not sure because it's taking forever to load the full diff.

[Alex Christensen]

Created attachment 438502 [details]
Patch

[Michael Catanzaro]

Comment on attachment 438502 [details]
Patch

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

> Source/WebCore/loader/FrameLoader.cpp:3598
> -    identifier = 0;
> +    identifier = { };
>      if (Page* page = m_frame.page()) {
> -        identifier = page->progress().createUniqueIdentifier();
> +        identifier = ResourceLoaderIdentifier::generate();
>          notifier().assignIdentifierToInitialRequest(identifier, m_documentLoader.get(), request);
>      }

This condition needs to be removed, because page is no longer required to generate the identifier.

[Michael Catanzaro]

Comment on attachment 438502 [details]
Patch

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

> Source/WebKit/WebProcess/InjectedBundle/InjectedBundlePageResourceLoadClient.h:61
> +    void didInitiateLoadForResource(WebPage&, WebFrame&, WebCore::ResourceLoaderIdentifier, const WebCore::ResourceRequest&, bool /*pageIsProvisionallyLoading*/) override;
> +    void willSendRequestForFrame(WebPage&, WebFrame&, WebCore::ResourceLoaderIdentifier, WebCore::ResourceRequest&, const WebCore::ResourceResponse&) override;
> +    void didReceiveResponseForResource(WebPage&, WebFrame&, WebCore::ResourceLoaderIdentifier, const WebCore::ResourceResponse&) override;
> +    void didReceiveContentLengthForResource(WebPage&, WebFrame&, WebCore::ResourceLoaderIdentifier, uint64_t contentLength) override;
> +    void didFinishLoadForResource(WebPage&, WebFrame&, WebCore::ResourceLoaderIdentifier) override;
> +    void didFailLoadForResource(WebPage&, WebFrame&, WebCore::ResourceLoaderIdentifier, const WebCore::ResourceError&) override;
> +    bool shouldCacheResponse(WebPage&, WebFrame&, WebCore::ResourceLoaderIdentifier) override;
> +    bool shouldUseCredentialStorage(WebPage&, WebFrame&, WebCore::ResourceLoaderIdentifier) override;

This is what's breaking the WPE/GTK EWS. It should work if you also update PageResourceLoadClient in WebKitWebPage.cpp. Should be trivial.

[Alex Christensen]

Created attachment 438510 [details]
Patch

[Michael Catanzaro]

Comment on attachment 438502 [details]
Patch

Ah, I notice there is also Source/WebKit/NetworkProcess/NetworkProcess.h.rej added my mistake.

[Alex Christensen]

(In reply to Michael Catanzaro from comment #29)
> Comment on attachment 438502 [details]
> Patch
> 
> Ah, I notice there is also Source/WebKit/NetworkProcess/NetworkProcess.h.rej
> added my mistake.

Oops.  I'll remove in my next iteration.

(In reply to Michael Catanzaro from comment #26)
> Comment on attachment 438502 [details]
> Patch
> 
> This condition needs to be removed, because page is no longer required to
> generate the identifier.

That may be true, but in a separate patch.  I think this should change as little as possible to help with bisection.

[Michael Catanzaro]

(In reply to Alex Christensen from comment #30)
> That may be true, but in a separate patch.  I think this should change as
> little as possible to help with bisection.

I mean it's really totally unused, I noticed because it introduces a -Wunused-variable warning. Don't do that. :P

[Darin Adler]

(In reply to Michael Catanzaro from comment #31)
> (In reply to Alex Christensen from comment #30)
> > That may be true, but in a separate patch.  I think this should change as
> > little as possible to help with bisection.
> 
> I mean it's really totally unused, I noticed because it introduces a
> -Wunused-variable warning. Don't do that. :P

So we can leave the null check in for now, and remove that in a later patch, but must not put it in a local variable.

[Alex Christensen]

Ohhh.  Now I see what you're talking about.  page is no longer needed.  Will remove the now-unnecessary branch.

[Alex Christensen]

Created attachment 438522 [details]
Patch

[Alex Christensen]

Created attachment 438523 [details]
Patch

[Alex Christensen]

Comment on attachment 438523 [details]
Patch

https://trac.webkit.org/r282712