Bug 209032 - Report all third party loads on a per-page basis
Summary: Report all third party loads on a per-page basis
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kate Cheney
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-03-12 18:30 PDT by Kate Cheney
Modified: 2020-03-13 12:44 PDT (History)
9 users (show)

See Also:


Attachments
Patch (43.19 KB, patch)
2020-03-12 18:44 PDT, Kate Cheney
no flags Details | Formatted Diff | Diff
Patch (42.79 KB, patch)
2020-03-12 19:06 PDT, Kate Cheney
no flags Details | Formatted Diff | Diff
Patch (50.76 KB, patch)
2020-03-13 08:25 PDT, Kate Cheney
no flags Details | Formatted Diff | Diff
Patch (50.88 KB, patch)
2020-03-13 09:58 PDT, Kate Cheney
no flags Details | Formatted Diff | Diff
Patch for landing (50.87 KB, patch)
2020-03-13 11:56 PDT, Kate Cheney
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kate Cheney 2020-03-12 18:30:36 PDT
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.
Comment 1 Kate Cheney 2020-03-12 18:31:07 PDT
<rdar://problem/60397323>
Comment 2 Kate Cheney 2020-03-12 18:44:38 PDT
Created attachment 393438 [details]
Patch
Comment 3 Kate Cheney 2020-03-12 19:06:37 PDT
Created attachment 393441 [details]
Patch
Comment 4 John Wilander 2020-03-12 19:53:21 PDT
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.
Comment 5 John Wilander 2020-03-12 19:54:49 PDT
Build failure in WebPage.cpp.
Comment 6 Kate Cheney 2020-03-13 08:09:04 PDT
(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.
Comment 7 Kate Cheney 2020-03-13 08:25:28 PDT
Created attachment 393483 [details]
Patch
Comment 8 Kate Cheney 2020-03-13 08:45:47 PDT
(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 :(
Comment 9 Chris Dumez 2020-03-13 09:02:08 PDT
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.
Comment 10 Chris Dumez 2020-03-13 09:08:31 PDT
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.
Comment 11 Kate Cheney 2020-03-13 09:19:27 PDT
(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
Comment 12 Chris Dumez 2020-03-13 09:22:56 PDT
(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.
Comment 13 Kate Cheney 2020-03-13 09:24:13 PDT
(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!
Comment 14 Kate Cheney 2020-03-13 09:58:01 PDT
Created attachment 393494 [details]
Patch
Comment 15 John Wilander 2020-03-13 10:48:43 PDT
(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!
Comment 16 Chris Dumez 2020-03-13 10:50:19 PDT
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.
Comment 17 Kate Cheney 2020-03-13 11:56:58 PDT
Created attachment 393513 [details]
Patch for landing
Comment 18 Kate Cheney 2020-03-13 11:57:30 PDT
(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!
Comment 19 WebKit Commit Bot 2020-03-13 12:43:48 PDT
Comment on attachment 393513 [details]
Patch for landing

Clearing flags on attachment: 393513

Committed r258421: <https://trac.webkit.org/changeset/258421>
Comment 20 WebKit Commit Bot 2020-03-13 12:43:50 PDT
All reviewed patches have been landed.  Closing bug.
Comment 21 Radar WebKit Bug Importer 2020-03-13 12:44:17 PDT
<rdar://problem/60429255>