Bug 180728 – Storage Access API: Make DocumentLoader::willSendRequest() and WebFrameLoaderClient::detachedFromParent2() tell the network process to get rid of any sub frame access entries

• Component: WebKit2
• Status: RESOLVED FIXED

[John Wilander]

This is part 4 of making Storage Access API frame-specific.

[Radar WebKit Bug Importer]

<rdar://problem/36009288>

[youenn fablet]

Knowing whether a frame is loading should not probably in WebLoaderStrategy.
frame navigations may not hit the network: data URLs, service worker interception.

It might best be done when we are actually changing a frame document.
This is currently done for main frame documents to reset ICE candidate filtering: see Page::didChangeMainDocument. Maybe we should do the same for any iframe.

[John Wilander]

Created attachment 329970 [details]
Patch

[youenn fablet]

Comment on attachment 329970 [details]
Patch

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

> Source/WebCore/dom/Document.h:1702
> +    void setHasFrameSpecificStorageAccess(bool) const;

Does not seem right to be const for a setter.

> Source/WebCore/loader/DocumentLoader.cpp:537
> +        frameLoader()->client().dispatchWillChangeSubFrameDocument();

We will call this for every redirection. Is it really what we want? or do we want it for the initial load only?

> Source/WebCore/platform/network/cf/NetworkStorageSessionCFNet.cpp:338
> +    auto it1 = m_framesGrantedStorageAccess.find(frameID);

s/it1/iteration/

> Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:382
> +void WebFrameLoaderClient::dispatchWillChangeSubFrameDocument()

Maybe we should make it more abstract, something like dispatchWillChangeDocument().
Inside it, we have m_frame so we might know whether this is a main frame or not.
In the future, we might do some specific work when changing main frame document.

> Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:391
> +        m_hasFrameSpecificStorageAccess = false;

Can we have m_hasFrameSpecificStorageAccess == true and frameID() or pageID() being null?

[John Wilander]

Created attachment 329977 [details]
Patch

[John Wilander]

The latest patch only has a slight change to fix the build for non-recent Cocoa platforms.

[John Wilander]

Created attachment 329989 [details]
Patch

[John Wilander]

(In reply to youenn fablet from comment #4)
> Comment on attachment 329970 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=329970&action=review
> 
> > Source/WebCore/dom/Document.h:1702
> > +    void setHasFrameSpecificStorageAccess(bool) const;
> 
> Does not seem right to be const for a setter.

True. It doesn't change the Document object but I removed the const.

> > Source/WebCore/loader/DocumentLoader.cpp:537
> > +        frameLoader()->client().dispatchWillChangeSubFrameDocument();
> 
> We will call this for every redirection. Is it really what we want? or do we
> want it for the initial load only?

I think it's enough for the initial load for my purposes. I've added at check for redirect.

> > Source/WebCore/platform/network/cf/NetworkStorageSessionCFNet.cpp:338
> > +    auto it1 = m_framesGrantedStorageAccess.find(frameID);
> 
> s/it1/iteration/

Will fix.

> > Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:382
> > +void WebFrameLoaderClient::dispatchWillChangeSubFrameDocument()
> 
> Maybe we should make it more abstract, something like
> dispatchWillChangeDocument().
> Inside it, we have m_frame so we might know whether this is a main frame or
> not.
> In the future, we might do some specific work when changing main frame
> document.

Fixed.

> > Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:391
> > +        m_hasFrameSpecificStorageAccess = false;
> 
> Can we have m_hasFrameSpecificStorageAccess == true and frameID() or
> pageID() being null?

True. I removed the checks of frameID and pageID.

[EWS Watchlist]

Attachment 329989 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1197:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1207:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.h:124:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.h:125:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 4 in 49 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[John Wilander]

Style errors are all of the "Extra space before ( in function call" variety.

[John Wilander]

Created attachment 329992 [details]
Patch

[EWS Watchlist]

Attachment 329992 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1197:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1207:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.h:124:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.h:125:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 4 in 52 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[John Wilander]

Created attachment 329994 [details]
Patch

[EWS Watchlist]

Attachment 329994 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1197:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1207:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.h:124:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.h:125:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 4 in 54 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[youenn fablet]

Comment on attachment 329994 [details]
Patch

r=me if bots are happy.
It seems http/tests/storageAccess/request-storage-access-top-frame.html has some issues with Mac-wk2

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

> Source/WebCore/dom/Document.cpp:7560
> +void Document::setHasFrameSpecificStorageAccess(bool value)

If we add this setter, it might be better to add an hasFrameSpecificStorageAccess getter as well.
I also wonder whether setHasFrameSpecificStorageAccess is called in code that is not #ifdef by VE(CFNETWORK_STORAGE_PARTITIONING).
In that case, maybe the whole Document::setHasFrameSpecificStorageAccess should be #ifdef.

> Source/WebCore/loader/DocumentLoader.cpp:538
> +        frameLoader()->client().dispatchWillChangeDocument();

If there is no specific reason to call dispatchWillChangeDocument after the blocker checks, I would suggest adding this call this in startMainResourceLoad.

> Source/WebKit/UIProcess/WebPageProxy.h:1248
>      void hasStorageAccess(String&& subFrameHost, String&& topFrameHost, uint64_t frameID, uint64_t pageID, uint64_t webProcessContextId);

It seems strange to pass r-values for a method called hasXX.
Looking at the implementation, they can probably be changed to const String&.
For instance, Messages::WebPageProxy::HasStorageAccess is taking a const String&, not a String.
Ditto for requestStorageAccess.
Might be done as a follow-up or in this patch.

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1207
> +void WebsiteDataStore::removeStorageAccess(uint64_t frameID, uint64_t pageID)

I would have expected to have pageID before frameID, since frameID is specific to its pageID.

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1210
> +        processPool->networkProcess()->removeStorageAccess(m_sessionID, frameID, pageID);

This seems somehow strange that we go from WebProcess to UIProcess to NetworkProcess.
Why cannot we go from WebProcess to its NetworkProcess to remove the storage access there.
If needed, the WebProcess can also update the websitedatastore resource load statistics with another IPC call from WebProcess to UIProcess.
Is that possible to do that instead?

[EWS Watchlist]

Comment on attachment 329994 [details]
Patch

Attachment 329994 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/5784421

New failing tests:
http/tests/storageAccess/request-storage-access-top-frame.html

[EWS Watchlist]

Created attachment 329999 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[John Wilander]

(In reply to youenn fablet from comment #15)
> Comment on attachment 329994 [details]
> Patch
> 
> r=me if bots are happy.
> It seems http/tests/storageAccess/request-storage-access-top-frame.html has
> some issues with Mac-wk2

Yes, I will fix that. This test should no longer be marked as Pass for pre-High Sierra since all the logic is now behind the CFNETWORK_STORAGE_PARTITIONING check.

> View in context:
> https://bugs.webkit.org/attachment.cgi?id=329994&action=review
> 
> > Source/WebCore/dom/Document.cpp:7560
> > +void Document::setHasFrameSpecificStorageAccess(bool value)
> 
> If we add this setter, it might be better to add an
> hasFrameSpecificStorageAccess getter as well.

You are right. Will fix.

> I also wonder whether setHasFrameSpecificStorageAccess is called in code
> that is not #ifdef by VE(CFNETWORK_STORAGE_PARTITIONING).
> In that case, maybe the whole Document::setHasFrameSpecificStorageAccess
> should be #ifdef.

Correct. Will fix.

> > Source/WebCore/loader/DocumentLoader.cpp:538
> > +        frameLoader()->client().dispatchWillChangeDocument();
> 
> If there is no specific reason to call dispatchWillChangeDocument after the
> blocker checks, I would suggest adding this call this in
> startMainResourceLoad.

There is nothing called startMainResourceLoad in WebCore or WebKit. Typo? Anyway, we should discuss so that I understand what you mean. Perhaps what you're going for is that we shouldn't clear out storage access if for _any_ reason the load is blocked or cancelled? And that could be caught in some function that is only ever called when all block checks are done?

> > Source/WebKit/UIProcess/WebPageProxy.h:1248
> >      void hasStorageAccess(String&& subFrameHost, String&& topFrameHost, uint64_t frameID, uint64_t pageID, uint64_t webProcessContextId);
> 
> It seems strange to pass r-values for a method called hasXX.
> Looking at the implementation, they can probably be changed to const String&.
> For instance, Messages::WebPageProxy::HasStorageAccess is taking a const
> String&, not a String.
> Ditto for requestStorageAccess.
> Might be done as a follow-up or in this patch.

You are probably right. This code didn't change in this patch (some code was just moved in the file to group under CFNETWORK_STORAGE_PARTITIONING check). We can revisit.

> > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1207
> > +void WebsiteDataStore::removeStorageAccess(uint64_t frameID, uint64_t pageID)
> 
> I would have expected to have pageID before frameID, since frameID is
> specific to its pageID.

There is a reason but I agree this is an itch. The reason is that frameID is currently the primary key in the lookup table for storage access. But, I will probably refactor to make pageID the primary key to allow for efficient clear when a whole page goes away. If I do that I will also re-order the parameters.

> > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1210
> > +        processPool->networkProcess()->removeStorageAccess(m_sessionID, frameID, pageID);
> 
> This seems somehow strange that we go from WebProcess to UIProcess to
> NetworkProcess.
> Why cannot we go from WebProcess to its NetworkProcess to remove the storage
> access there.
> If needed, the WebProcess can also update the websitedatastore resource load
> statistics with another IPC call from WebProcess to UIProcess.
> Is that possible to do that instead?

This code did not change in this patch but I agree. If nothing else, for race reasons. The reason why the UI process is involved at all is that it makes the decision if storage access should be allowed and also persists statistics on these calls. But it could absolutely be split into two calls. Will address in a follow-up.

[John Wilander]

Created attachment 330038 [details]
Patch

[John Wilander]

Comment on attachment 330038 [details]
Patch

Already reviewed. Just want EWS feedback on the small changes Youenn wanted.

[EWS Watchlist]

Attachment 330038 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1197:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1207:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.h:124:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.h:125:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 4 in 54 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[John Wilander]

Comment on attachment 330038 [details]
Patch

Thanks for the review, Youenn!

[WebKit Commit Bot]

Comment on attachment 330038 [details]
Patch

Clearing flags on attachment: 330038

Committed r226235: <https://trac.webkit.org/changeset/226235>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

http/tests/storageAccess/request-and-grant-storage-access-cross-origin-sandboxed-iframe-from-prevalent-domain-with-non-recent-user-interaction-and-try-access-from-right-frame.html is failing after this change:
https://build.webkit.org/results/Apple%20High%20Sierra%20Release%20WK2%20(Tests)/r226240%20(1917)/results.html

[John Wilander]

(In reply to Ryan Haddad from comment #25)
> http/tests/storageAccess/request-and-grant-storage-access-cross-origin-
> sandboxed-iframe-from-prevalent-domain-with-non-recent-user-interaction-and-
> try-access-from-right-frame.html is failing after this change:
> https://build.webkit.org/results/
> Apple%20High%20Sierra%20Release%20WK2%20(Tests)/r226240%20(1917)/results.html

Thanks, Ryan! I skipped the test for now in https://bugs.webkit.org/show_bug.cgi?id=181108. Will revisit to see why it fails on the bots but not for me locally.