Bug 215595 – Third party domains are not stored in the case of back/forward navigations

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Kate Cheney]

We should keep per-page loaded third party domains somewhere for when we navigate back/forward.

[Kate Cheney]

<rdar://problem/66642893>

[Kate Cheney]

Created attachment 406756 [details]
Patch

[Darin Adler]

Comment on attachment 406756 [details]
Patch

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

Should upload a new version to fix the Windows build.

> Source/WebCore/history/CachedPage.h:40
> +    explicit CachedPage(Page&, Vector<RegistrableDomain>&&);

Most of the benefit of the explicit keyword is gone now that this has two arguments, so we could remove it.

A bit strange that we have to pass this explicitly here. This seems to possibly indicate a problem with the design pattern.

[Kate Cheney]

(In reply to Darin Adler from comment #3)
> Comment on attachment 406756 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=406756&action=review
> 
> Should upload a new version to fix the Windows build.

Will do.

> 
> > Source/WebCore/history/CachedPage.h:40
> > +    explicit CachedPage(Page&, Vector<RegistrableDomain>&&);
> 
> Most of the benefit of the explicit keyword is gone now that this has two
> arguments, so we could remove it.
> 
> A bit strange that we have to pass this explicitly here. This seems to
> possibly indicate a problem with the design pattern.

I can probably change it to use a setter instead.

[Chris Dumez]

Comment on attachment 406756 [details]
Patch

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

> Source/WebCore/loader/FrameLoader.cpp:2005
> +        BackForwardCache::singleton().addIfCacheable(*history().currentItem(), m_frame.page(), m_frame.loader().client().loadedThirdPartyDomainsForCache());

Why do we need to pass the domain here at the call site. Cannot the CachedPage constructor simply call m_frame.loader().client().loadedThirdPartyDomainsForCache() itself if caching does end up happening?

> Source/WebCore/loader/FrameLoader.cpp:2054
> +        m_client->restoreThirdPartyDomains(cachedPage->loadedThirdPartyDomains());

May be nicer to have this inside CachedPage so that all the logic for saving and restoring is inside CachedPage.

> Source/WebCore/loader/FrameLoaderClient.h:377
> +    virtual void restoreThirdPartyDomains(Vector<RegistrableDomain>&) { }

Odd to pass a Vector<RegistrableDomain>& here. You probably want a Vector<RegistrableDomain>&&

Come to think of it though, do we really need a new client function? Cannot we simply replay and call didLoadFromRegistrableDomain() in a loop?

[Kate Cheney]

(In reply to Chris Dumez from comment #5)
> Comment on attachment 406756 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=406756&action=review
> 
> > Source/WebCore/loader/FrameLoader.cpp:2005
> > +        BackForwardCache::singleton().addIfCacheable(*history().currentItem(), m_frame.page(), m_frame.loader().client().loadedThirdPartyDomainsForCache());
> 
> Why do we need to pass the domain here at the call site. Cannot the
> CachedPage constructor simply call
> m_frame.loader().client().loadedThirdPartyDomainsForCache() itself if
> caching does end up happening?
> 

Yes, good idea.

> > Source/WebCore/loader/FrameLoader.cpp:2054
> > +        m_client->restoreThirdPartyDomains(cachedPage->loadedThirdPartyDomains());
> 
> May be nicer to have this inside CachedPage so that all the logic for saving
> and restoring is inside CachedPage.
> 
> > Source/WebCore/loader/FrameLoaderClient.h:377
> > +    virtual void restoreThirdPartyDomains(Vector<RegistrableDomain>&) { }
> 
> Odd to pass a Vector<RegistrableDomain>& here. You probably want a
> Vector<RegistrableDomain>&&
> 
> Come to think of it though, do we really need a new client function? Cannot
> we simply replay and call didLoadFromRegistrableDomain() in a loop?

Good point, I will remove this function.

[Kate Cheney]

Created attachment 406795 [details]
Patch

[Chris Dumez]

Comment on attachment 406795 [details]
Patch

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

> Source/WebCore/loader/FrameLoader.cpp:2055
> +        for (auto& domain : cachedPage->loadedThirdPartyDomains())

Why didn't you move this inside CachedPage::restore() like I suggested? We want cache / restore logic inside CachedPage / CachedFrame as much as possible.

[Chris Dumez]

Comment on attachment 406795 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/WKWebViewPrivate.h:355
> +- (void)_setloadedThirdPartyDomain:(NSString *)domain completionHandler:(void (^)(void))completionHandler WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));

I don't think "set" in the name is great, given what the method actually does. I think we should use "add" prefix instead. Also, I like when we use "ForTesting" suffix for such things to make it clear what it is for.

> Source/WebKit/WebProcess/WebPage/WebPage.h:1218
> +    Vector<WebCore::RegistrableDomain> loadedThirdPartyDomainsForCache();

Not sure we need the "ForCache" suffix here. Also, this getter should be const. Finally, I think this should return a 'const HashSet<RegistrableDomain>&' since this is the most efficient. The call site can call copyToVector() as needed.

> Source/WebKit/WebProcess/WebPage/WebPage.h:1219
> +    void setloadedThirdPartyDomain(WebCore::RegistrableDomain&&, CompletionHandler<void()>&&);

addLoadedThirdPartyDomainForTesting

> Tools/TestWebKitAPI/Tests/WebKitCocoa/ResourceLoadStatistics.mm:1664
> +    [webView _setloadedThirdPartyDomain:@"example1.com" completionHandler: ^(void) {

I don't even think we need this setter. Why can't you simply actually load different domains in the view? At least it would be a real test. Now it is half faked.
See ProcessSwapOnNavigation.mm tests to see how you can load different registrable domains in tests.

[Sam Weinig]

Comment on attachment 406795 [details]
Patch

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

> Source/WebCore/history/CachedPage.h:62
> +    Vector<RegistrableDomain>& loadedThirdPartyDomains() { return m_loadedThirdPartyDomains; }

Can we give this a clearer name? I don't know from looking at this what this represents?

[Kate Cheney]

(In reply to Chris Dumez from comment #9)
> Comment on attachment 406795 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=406795&action=review
> 
> > Source/WebKit/UIProcess/API/Cocoa/WKWebViewPrivate.h:355
> > +- (void)_setloadedThirdPartyDomain:(NSString *)domain completionHandler:(void (^)(void))completionHandler WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));
> 
> I don't think "set" in the name is great, given what the method actually
> does. I think we should use "add" prefix instead. Also, I like when we use
> "ForTesting" suffix for such things to make it clear what it is for.
> 

Ok -- can remove this after changing the test probably.

> > Source/WebKit/WebProcess/WebPage/WebPage.h:1218
> > +    Vector<WebCore::RegistrableDomain> loadedThirdPartyDomainsForCache();
> 
> Not sure we need the "ForCache" suffix here. Also, this getter should be
> const. Finally, I think this should return a 'const
> HashSet<RegistrableDomain>&' since this is the most efficient. The call site
> can call copyToVector() as needed.
> 

I used ForCache because there is already a loadedThirdPartyDomains() function in WebPage.cpp, so the compiler was unhappy. I will change it to be const and return const HashSet<RegistrableDomain>& though. I can change the ForCache name if you have a suggestion.

> > Source/WebKit/WebProcess/WebPage/WebPage.h:1219
> > +    void setloadedThirdPartyDomain(WebCore::RegistrableDomain&&, CompletionHandler<void()>&&);
> 
> addLoadedThirdPartyDomainForTesting
> 
> > Tools/TestWebKitAPI/Tests/WebKitCocoa/ResourceLoadStatistics.mm:1664
> > +    [webView _setloadedThirdPartyDomain:@"example1.com" completionHandler: ^(void) {
> 
> I don't even think we need this setter. Why can't you simply actually load
> different domains in the view? At least it would be a real test. Now it is
> half faked.
> See ProcessSwapOnNavigation.mm tests to see how you can load different
> registrable domains in tests.

Will change.

[Chris Dumez]

(In reply to katherine_cheney from comment #11)
> (In reply to Chris Dumez from comment #9)
> > Comment on attachment 406795 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=406795&action=review
> > 
> > > Source/WebKit/UIProcess/API/Cocoa/WKWebViewPrivate.h:355
> > > +- (void)_setloadedThirdPartyDomain:(NSString *)domain completionHandler:(void (^)(void))completionHandler WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));
> > 
> > I don't think "set" in the name is great, given what the method actually
> > does. I think we should use "add" prefix instead. Also, I like when we use
> > "ForTesting" suffix for such things to make it clear what it is for.
> > 
> 
> Ok -- can remove this after changing the test probably.
> 
> > > Source/WebKit/WebProcess/WebPage/WebPage.h:1218
> > > +    Vector<WebCore::RegistrableDomain> loadedThirdPartyDomainsForCache();
> > 
> > Not sure we need the "ForCache" suffix here. Also, this getter should be
> > const. Finally, I think this should return a 'const
> > HashSet<RegistrableDomain>&' since this is the most efficient. The call site
> > can call copyToVector() as needed.
> > 
> 
> I used ForCache because there is already a loadedThirdPartyDomains()
> function in WebPage.cpp, so the compiler was unhappy. I will change it to be
> const and return const HashSet<RegistrableDomain>& though. I can change the
> ForCache name if you have a suggestion.

Maybe we can rename the existing one (which is used for IPC and has a completion handler) to something like: requestLoadedThirdPartyDomains

> 
> > > Source/WebKit/WebProcess/WebPage/WebPage.h:1219
> > > +    void setloadedThirdPartyDomain(WebCore::RegistrableDomain&&, CompletionHandler<void()>&&);
> > 
> > addLoadedThirdPartyDomainForTesting
> > 
> > > Tools/TestWebKitAPI/Tests/WebKitCocoa/ResourceLoadStatistics.mm:1664
> > > +    [webView _setloadedThirdPartyDomain:@"example1.com" completionHandler: ^(void) {
> > 
> > I don't even think we need this setter. Why can't you simply actually load
> > different domains in the view? At least it would be a real test. Now it is
> > half faked.
> > See ProcessSwapOnNavigation.mm tests to see how you can load different
> > registrable domains in tests.
> 
> Will change.

[Kate Cheney]

(In reply to Sam Weinig from comment #10)
> Comment on attachment 406795 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=406795&action=review
> 
> > Source/WebCore/history/CachedPage.h:62
> > +    Vector<RegistrableDomain>& loadedThirdPartyDomains() { return m_loadedThirdPartyDomains; }
> 
> Can we give this a clearer name? I don't know from looking at this what this
> represents?

I would be open to that if you think it is unclear. It represents a list of third party domains that have been loaded on a page via resources/iframes, etc. Other possible ideas: loadedCrossSiteDomains, loadedSubResourceDomains. Let me know if you have other ideas!

[Darin Adler]

(In reply to Chris Dumez from comment #12)
> Maybe we can rename the existing one (which is used for IPC and has a
> completion handler) to something like: requestLoadedThirdPartyDomains

Or even getXXX. This is part of why we reserve the word get; I think it’s good for completion handler based asynchronous functions as well as ones with out arguments.

[Kate Cheney]

Created attachment 406862 [details]
Patch

[Kate Cheney]

Created attachment 406863 [details]
Patch

[Chris Dumez]

Comment on attachment 406862 [details]
Patch

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

> Source/WebCore/loader/FrameLoaderClient.h:378
> +    virtual const HashSet<RegistrableDomain> loadedThirdPartyDomains() const { return { }; }

const without & is not useful. This should either return a 'HashSet<RegistrableDomain>' or a 'const HashSet<RegistrableDomain>&'. Likely 'HashSet<RegistrableDomain>' here because of this override and this is not performance sensitive code.
Note that maybe FrameLoaderClient::loadedThirdPartyDomains() could return a Vector<RegistrableDomain> (WebPage:: loadedThirdPartyDomains() would keep returning a const HashSet<RegistrableDomain>& though), since we need to make a copy anyway and we end up needing a Vector.

Right now we go from HashSet -> HashSet copy -> Vector.
Ideally, we'd go from HashSet -> Vector directly.

[Kate Cheney]

(In reply to Chris Dumez from comment #17)
> Comment on attachment 406862 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=406862&action=review
> 
> > Source/WebCore/loader/FrameLoaderClient.h:378
> > +    virtual const HashSet<RegistrableDomain> loadedThirdPartyDomains() const { return { }; }
> 
> const without & is not useful. This should either return a
> 'HashSet<RegistrableDomain>' or a 'const HashSet<RegistrableDomain>&'.
> Likely 'HashSet<RegistrableDomain>' here because of this override and this
> is not performance sensitive code.
> Note that maybe FrameLoaderClient::loadedThirdPartyDomains() could return a
> Vector<RegistrableDomain> (WebPage:: loadedThirdPartyDomains() would keep
> returning a const HashSet<RegistrableDomain>& though), since we need to make
> a copy anyway and we end up needing a Vector.
> 
> Right now we go from HashSet -> HashSet copy -> Vector.
> Ideally, we'd go from HashSet -> Vector directly.

Ahh smart, I see. Thanks for explaining, will fix.

[Kate Cheney]

Created attachment 406865 [details]
Patch

[Sam Weinig]

(In reply to katherine_cheney from comment #13)
> (In reply to Sam Weinig from comment #10)
> > Comment on attachment 406795 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=406795&action=review
> > 
> > > Source/WebCore/history/CachedPage.h:62
> > > +    Vector<RegistrableDomain>& loadedThirdPartyDomains() { return m_loadedThirdPartyDomains; }
> > 
> > Can we give this a clearer name? I don't know from looking at this what this
> > represents?
> 
> I would be open to that if you think it is unclear. It represents a list of
> third party domains that have been loaded on a page via resources/iframes,
> etc. Other possible ideas: loadedCrossSiteDomains, loadedSubResourceDomains.
> Let me know if you have other ideas!

So its the set of domains that the page (including any subframes) has loaded any resources from? Does this include all subresource types? (e.g. iframes, images, scripts, etc?)

[Kate Cheney]

(In reply to Sam Weinig from comment #20)
> (In reply to katherine_cheney from comment #13)
> > (In reply to Sam Weinig from comment #10)
> > > Comment on attachment 406795 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=406795&action=review
> > > 
> > > > Source/WebCore/history/CachedPage.h:62
> > > > +    Vector<RegistrableDomain>& loadedThirdPartyDomains() { return m_loadedThirdPartyDomains; }
> > > 
> > > Can we give this a clearer name? I don't know from looking at this what this
> > > represents?
> > 
> > I would be open to that if you think it is unclear. It represents a list of
> > third party domains that have been loaded on a page via resources/iframes,
> > etc. Other possible ideas: loadedCrossSiteDomains, loadedSubResourceDomains.
> > Let me know if you have other ideas!
> 
> So its the set of domains that the page (including any subframes) has loaded
> any resources from? Does this include all subresource types? (e.g. iframes,
> images, scripts, etc?)

Yes, as long as the domain is not the same as the first party domain.

[Darin Adler]

I think we really want the word "subresource" in there. (One word, not two words "sub resource".)

[Kate Cheney]

(In reply to Darin Adler from comment #22)
> I think we really want the word "subresource" in there. (One word, not two
> words "sub resource".)

Ok. Maybe loadedSubresourceDomains, or even just subresourceDomains?

[Chris Dumez]

Comment on attachment 406865 [details]
Patch

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

r=me with nit fix. Please address naming issue raised by Darin & Sam before landing too.

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:6836
> +const HashSet<WebCore::RegistrableDomain>& WebPage::loadedThirdPartyDomains() const

This could be inlined in the header given how trivial it is.

[Kate Cheney]

Created attachment 406888 [details]
Patch for landing

[Kate Cheney]

This patch changes the name from loadedThirdPartyDomains to loadedSubresourceDomains based on the comments on this thread. Sam or Darin, if you have more thoughts on the name I can post a followup patch.

[Darin Adler]

(In reply to katherine_cheney from comment #26)
> This patch changes the name from loadedThirdPartyDomains to
> loadedSubresourceDomains based on the comments on this thread. Sam or Darin,
> if you have more thoughts on the name I can post a followup patch.

I like that name. I think being explicit about "these are the domains all the subresources are in" but implicit about "but we leave out the first party domain" is the right call. A comment somewhere could state the latter rule if we think someone will be confused about it.

[EWS]

Committed r265916: <https://trac.webkit.org/changeset/265916>

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