Bug 180728 - Storage Access API: Make DocumentLoader::willSendRequest() and WebFrameLoaderClient::detachedFromParent2() tell the network process to get rid of any sub frame access entries
Summary: Storage Access API: Make DocumentLoader::willSendRequest() and WebFrameLoader...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: John Wilander
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-12-12 16:51 PST by John Wilander
Modified: 2017-12-21 16:59 PST (History)
11 users (show)

See Also:


Attachments
Patch (67.13 KB, patch)
2017-12-20 17:26 PST, John Wilander
no flags Details | Formatted Diff | Diff
Patch (67.08 KB, patch)
2017-12-20 17:39 PST, John Wilander
no flags Details | Formatted Diff | Diff
Patch (70.50 KB, patch)
2017-12-20 18:34 PST, John Wilander
no flags Details | Formatted Diff | Diff
Patch (73.25 KB, patch)
2017-12-20 18:54 PST, John Wilander
no flags Details | Formatted Diff | Diff
Patch (75.12 KB, patch)
2017-12-20 19:04 PST, John Wilander
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews105 for mac-elcapitan-wk2 (2.51 MB, application/zip)
2017-12-20 20:24 PST, Build Bot
no flags Details
Patch (76.32 KB, patch)
2017-12-21 10:40 PST, John Wilander
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description John Wilander 2017-12-12 16:51:41 PST
This is part 4 of making Storage Access API frame-specific.
Comment 1 Radar WebKit Bug Importer 2017-12-12 16:52:34 PST
<rdar://problem/36009288>
Comment 2 youenn fablet 2017-12-20 13:05:38 PST
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.
Comment 3 John Wilander 2017-12-20 17:26:28 PST
Created attachment 329970 [details]
Patch
Comment 4 youenn fablet 2017-12-20 17:34:36 PST
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?
Comment 5 John Wilander 2017-12-20 17:39:20 PST
Created attachment 329977 [details]
Patch
Comment 6 John Wilander 2017-12-20 17:41:13 PST
The latest patch only has a slight change to fix the build for non-recent Cocoa platforms.
Comment 7 John Wilander 2017-12-20 18:34:42 PST
Created attachment 329989 [details]
Patch
Comment 8 John Wilander 2017-12-20 18:36:38 PST
(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.
Comment 9 Build Bot 2017-12-20 18:36:39 PST
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.
Comment 10 John Wilander 2017-12-20 18:37:12 PST
Style errors are all of the "Extra space before ( in function call" variety.
Comment 11 John Wilander 2017-12-20 18:54:14 PST
Created attachment 329992 [details]
Patch
Comment 12 Build Bot 2017-12-20 18:55:31 PST
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.
Comment 13 John Wilander 2017-12-20 19:04:07 PST
Created attachment 329994 [details]
Patch
Comment 14 Build Bot 2017-12-20 19:06:35 PST
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.
Comment 15 youenn fablet 2017-12-20 20:03:02 PST
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?
Comment 16 Build Bot 2017-12-20 20:24:50 PST
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
Comment 17 Build Bot 2017-12-20 20:24:51 PST
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
Comment 18 John Wilander 2017-12-21 10:09:54 PST
(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.
Comment 19 John Wilander 2017-12-21 10:40:18 PST
Created attachment 330038 [details]
Patch
Comment 20 John Wilander 2017-12-21 10:40:53 PST
Comment on attachment 330038 [details]
Patch

Already reviewed. Just want EWS feedback on the small changes Youenn wanted.
Comment 21 Build Bot 2017-12-21 10:43:50 PST
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.
Comment 22 John Wilander 2017-12-21 11:35:46 PST
Comment on attachment 330038 [details]
Patch

Thanks for the review, Youenn!
Comment 23 WebKit Commit Bot 2017-12-21 11:56:49 PST
Comment on attachment 330038 [details]
Patch

Clearing flags on attachment: 330038

Committed r226235: <https://trac.webkit.org/changeset/226235>
Comment 24 WebKit Commit Bot 2017-12-21 11:56:51 PST
All reviewed patches have been landed.  Closing bug.
Comment 25 Ryan Haddad 2017-12-21 16:21:31 PST
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
Comment 26 John Wilander 2017-12-21 16:59:29 PST
(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.