Bug 209032 – Report all third party loads on a per-page basis

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

[Kate Cheney]

WKWebsiteDataStore should have an SPI call to report all third party domains loaded for a specific WebView. This should replace the call reporting only prevalent domains.

[Kate Cheney]

<rdar://problem/60397323>

[Kate Cheney]

Created attachment 393438 [details]
Patch

[Kate Cheney]

Created attachment 393441 [details]
Patch

[John Wilander]

Comment on attachment 393441 [details]
Patch

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

Bots seem unhappy.

> Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStore.mm:393
> +- (void)_getLoadedDomainsFor:(WKWebView *)webView completionHandler:(void (^)(NSArray<NSString *> *domains))completionHandler

If this truly returns third-party loads I think the name should include that, _getLoadedThirdPartyDomains(). Likewise for the rest of the re-naming.

> Tools/WebKitTestRunner/InjectedBundle/Bindings/TestRunner.idl:348
> +    void getLoadedDomains(object callback);

Likewise for re-naming in Tools code.

[John Wilander]

Build failure in WebPage.cpp.

[Kate Cheney]

(In reply to John Wilander from comment #5)
> Build failure in WebPage.cpp.

(In reply to John Wilander from comment #4)
> Comment on attachment 393441 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=393441&action=review
> 
> Bots seem unhappy.
> 

Whoops, not sure why that wasn't caught by my local build

> > Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStore.mm:393
> > +- (void)_getLoadedDomainsFor:(WKWebView *)webView completionHandler:(void (^)(NSArray<NSString *> *domains))completionHandler
> 
> If this truly returns third-party loads I think the name should include
> that, _getLoadedThirdPartyDomains(). Likewise for the rest of the re-naming.
> 
> > Tools/WebKitTestRunner/InjectedBundle/Bindings/TestRunner.idl:348
> > +    void getLoadedDomains(object callback);
> 
> Likewise for re-naming in Tools code.

Good call, uploading a new patch with this change.

[Kate Cheney]

Created attachment 393483 [details]
Patch

[Kate Cheney]

(In reply to katherine_cheney from comment #7)
> Created attachment 393483 [details]
> Patch

Style bot always seems upset when adding a function to Source/WebCore/loader/FrameLoaderClient.h :(

[Chris Dumez]

Comment on attachment 393483 [details]
Patch

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

> Source/WebKit/WebProcess/WebPage/WebPage.h:1192
> +    void getLoadedThirdPartyDomains(CompletionHandler<void(Vector<WebCore::RegistrableDomain>)>&&);

No get prefixes for getters please.

[Chris Dumez]

Comment on attachment 393483 [details]
Patch

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

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:6700
> +    m_loadedThirdPartyDomains.add(domain);

Where in your code are you checking that it is a third-party domain? As far as I can tell, you are adding the first-party domain to this set too.

[Kate Cheney]

(In reply to Chris Dumez from comment #10)
> Comment on attachment 393483 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=393483&action=review
> 
> > Source/WebKit/WebProcess/WebPage/WebPage.cpp:6700
> > +    m_loadedThirdPartyDomains.add(domain);
> 
> Where in your code are you checking that it is a third-party domain? As far
> as I can tell, you are adding the first-party domain to this set too.

You're right. I think I need to add a check for if the mainFrameURL == targetURL

[Chris Dumez]

(In reply to katherine_cheney from comment #11)
> (In reply to Chris Dumez from comment #10)
> > Comment on attachment 393483 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=393483&action=review
> > 
> > > Source/WebKit/WebProcess/WebPage/WebPage.cpp:6700
> > > +    m_loadedThirdPartyDomains.add(domain);
> > 
> > Where in your code are you checking that it is a third-party domain? As far
> > as I can tell, you are adding the first-party domain to this set too.
> 
> You're right. I think I need to add a check for if the mainFrameURL ==
> targetURL

I believe you want to be checking registrable domains, not URLs.

[Kate Cheney]

(In reply to Chris Dumez from comment #12)
> (In reply to katherine_cheney from comment #11)
> > (In reply to Chris Dumez from comment #10)
> > > Comment on attachment 393483 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=393483&action=review
> > > 
> > > > Source/WebKit/WebProcess/WebPage/WebPage.cpp:6700
> > > > +    m_loadedThirdPartyDomains.add(domain);
> > > 
> > > Where in your code are you checking that it is a third-party domain? As far
> > > as I can tell, you are adding the first-party domain to this set too.
> > 
> > You're right. I think I need to add a check for if the mainFrameURL ==
> > targetURL
> 
> I believe you want to be checking registrable domains, not URLs.

Yes, typo. I will be comparing the RegistrableDomains in WebPage::addLoadedThirdPartyDomain before adding to the HashSet. Thank you for pointing this out!

[Kate Cheney]

Created attachment 393494 [details]
Patch

[John Wilander]

(In reply to katherine_cheney from comment #11)
> (In reply to Chris Dumez from comment #10)
> > Comment on attachment 393483 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=393483&action=review
> > 
> > > Source/WebKit/WebProcess/WebPage/WebPage.cpp:6700
> > > +    m_loadedThirdPartyDomains.add(domain);
> > 
> > Where in your code are you checking that it is a third-party domain? As far
> > as I can tell, you are adding the first-party domain to this set too.
> 
> You're right. I think I need to add a check for if the mainFrameURL ==
> targetURL

I actually thought about this but reckoned that it must be checked already since the same bug would otherwise apply to existing code with the isPrevalent check. Apparently we do have that bug in trunk then. Good catch, Chris!

[Chris Dumez]

Comment on attachment 393494 [details]
Patch

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

> Source/WebCore/Modules/websockets/WebSocket.cpp:331
> +            frame->loader().client().addLoadedThirdPartyDomain(WTFMove(domain));

You're calling addLoadedThirdPartyDomain() with a domain that is not necessarily third-party. As a result, I think this method is badly named. Maybe something like FrameLoaderClient::didLoadFromRegistrableDomain(), very generic and then it is up to the client implementation to decide what to do with it.

[Kate Cheney]

Created attachment 393513 [details]
Patch for landing

[Kate Cheney]

(In reply to Chris Dumez from comment #16)
> Comment on attachment 393494 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=393494&action=review
> 
> > Source/WebCore/Modules/websockets/WebSocket.cpp:331
> > +            frame->loader().client().addLoadedThirdPartyDomain(WTFMove(domain));
> 
> You're calling addLoadedThirdPartyDomain() with a domain that is not
> necessarily third-party. As a result, I think this method is badly named.
> Maybe something like FrameLoaderClient::didLoadFromRegistrableDomain(), very
> generic and then it is up to the client implementation to decide what to do
> with it.

Made these changes, thanks Chris!

[WebKit Commit Bot]

Comment on attachment 393513 [details]
Patch for landing

Clearing flags on attachment: 393513

Committed r258421: <https://trac.webkit.org/changeset/258421>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/60429255>