Bug 175759 – Introduce Storage Access API (document parts) as an experimental feature

• Component: WebKit2
• Status: RESOLVED FIXED

[John Wilander]

An experimental feature which allows cross-origin iframes to request access to their first-party storage (as opposed to partitioned storage). This might be restricted to cookies or might cover all stateful mechanisms.

[John Wilander]

<rdar://problem/33666847>

[John Wilander]

Created attachment 318613 [details]
Patch

[Build Bot]

Comment on attachment 318613 [details]
Patch

Attachment 318613 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4351940

New failing tests:
http/tests/loading/resourceLoadStatistics/request-storage-access-cross-origin-sandboxed-iframe-with-unique-origin.html
http/tests/loading/resourceLoadStatistics/request-storage-access-same-origin-iframe.html
http/tests/loading/resourceLoadStatistics/request-and-deny-storage-access-cross-origin-iframe.html
http/tests/loading/resourceLoadStatistics/request-and-grant-storage-access-cross-origin-sandboxed-iframe.html
http/tests/loading/resourceLoadStatistics/request-storage-access-same-origin-sandboxed-iframe.html
http/tests/loading/resourceLoadStatistics/request-and-grant-storage-access-cross-origin-iframe.html
http/tests/loading/resourceLoadStatistics/request-and-deny-storage-access-cross-origin-sandboxed-iframe.html
http/tests/loading/resourceLoadStatistics/request-storage-access-cross-origin-sandboxed-iframe-without-allow-token.html
http/tests/loading/resourceLoadStatistics/request-storage-access-same-origin-sandboxed-iframe-without-allow-token.html

[Build Bot]

Created attachment 318615 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Build Bot]

Comment on attachment 318613 [details]
Patch

Attachment 318613 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4352386

New failing tests:
http/tests/loading/resourceLoadStatistics/request-storage-access-cross-origin-sandboxed-iframe-with-unique-origin.html
http/tests/loading/resourceLoadStatistics/request-storage-access-same-origin-iframe.html
http/tests/loading/resourceLoadStatistics/request-and-deny-storage-access-cross-origin-iframe.html
http/tests/loading/resourceLoadStatistics/request-and-grant-storage-access-cross-origin-sandboxed-iframe.html
http/tests/loading/resourceLoadStatistics/request-storage-access-same-origin-sandboxed-iframe.html
http/tests/loading/resourceLoadStatistics/request-and-grant-storage-access-cross-origin-iframe.html
http/tests/loading/resourceLoadStatistics/request-and-deny-storage-access-cross-origin-sandboxed-iframe.html
http/tests/loading/resourceLoadStatistics/request-storage-access-cross-origin-sandboxed-iframe-without-allow-token.html
http/tests/loading/resourceLoadStatistics/request-storage-access-same-origin-sandboxed-iframe-without-allow-token.html

[Build Bot]

Created attachment 318620 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Andy Estes]

Comment on attachment 318613 [details]
Patch

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

Not a full review, but I do have a few comments.

> Source/WebCore/dom/Document.cpp:4990
> +    if (!Settings::storageAccessAPIEnabled())
> +        return false;

This isn't a great way to disable this feature at runtime. In script, you want "window.requestStorageAccess" to be undefined with the feature disabled, but it won't be when implemented like this. The right way to do this is to annotate your methods with "EnabledBySetting" in the IDL file.

> Source/WebCore/dom/Document.cpp:5012
> +    if (!UserGestureIndicator::processingUserGesture())
> +        return false;
> +
> +    if (isSandboxed(SandboxStorageAccessByUserActivation))
> +        return false;

These checks seem too late. If I call this once from the main frame without a user gesture, then m_hasStorageAccess will be set to true but the function will return false. If I then call it a second time (still from the main frame without a user gesture), won't it then return true due to the "if (m_frame->isMainFrame())" check above?

> Source/WebCore/dom/SecurityContext.cpp:89
> +    static const char* const supportedPolicies[] = {
> +        "allow-forms", "allow-same-origin", "allow-scripts", "allow-top-navigation", "allow-pointer-lock", "allow-popups", "allow-popups-to-escape-sandbox", "allow-top-navigation-by-user-activation", "allow-storage-access-by-user-activation"

Is it ok to define new sandbox flags that aren't specified by HTML? Is this flag being added to a spec?

> Source/WebCore/loader/ResourceLoadObserver.cpp:294
> +    if (!m_storageAccessMap.contains(subFrameTopPrivatelyControlledDomain)) {
> +        HashSet<String> topDomainEntry { topFrameTopPrivatelyControlledDomain };
> +        m_storageAccessMap.set(subFrameTopPrivatelyControlledDomain, WTFMove(topDomainEntry));
> +    } else if (m_storageAccessMap.get(subFrameTopPrivatelyControlledDomain).contains(topFrameTopPrivatelyControlledDomain))
> +        return;

You should use HashMap::add() with an empty value, then check the AddResult. If "isNewEntry" is true, then you can use the iterator to set a new value. Otherwise, you can check the iterator to see if it contains topFrameTopPrivatelyControlledDomain.

[John Wilander]

Created attachment 318645 [details]
Patch

[John Wilander]

Thanks, Andy! I uploaded a new patch to address the test failures but I'll look into your comments now.

[Andy Estes]

(In reply to Andy Estes from comment #7)
> you want "window.requestStorageAccess" to be undefined with the feature disabled

"document.requestStorageAccess", that is.

[Brent Fulgham]

Comment on attachment 318645 [details]
Patch

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

This is a great start, but let's handle the experimental feature the right way from the start. r- to correct that. :-)

> Source/JavaScriptCore/Configurations/FeatureDefines.xcconfig:157
> +ENABLE_STORAGE_ACCESS_API = ENABLE_STORAGE_ACCESS_API;

I don't think we want to add more "compile-time" guards like this for experimental features. We want to expose them as runtime-configurable switches we can control through the User Agent application (e.g., Safari's Developer menu). This doesn't seem like a feature that requires special back-end support or other code that needs to be compiled out for WebKit ports that are missing some critical platform feature. I suggest getting rid of this feature definition entirely.

> Source/WebCore/dom/Document.cpp:5014
> +    String returnValue;

This doesn't seem to be used.

> Source/WebCore/dom/Document.idl:125
> +    [Conditional=STORAGE_ACCESS_API] boolean requestStorageAccess();

As Andy pointed out in his review, this should be "EnabledBySetting=StorageAccessAPI". Then update "WebKit/Shared/WebPreferencesDefinition.h" and add "StorageAccessAPI" to the FOR_EACH_WEBKIT_EXPERIMENTAL_FEATURE_PREFERENCE macro. Take a look at SubresourceIntegrity/SubresourceIntegrityEnabled to see the right way to do this.

> Source/WebCore/dom/SecurityContext.cpp:89
> +        "allow-forms", "allow-same-origin", "allow-scripts", "allow-top-navigation", "allow-pointer-lock", "allow-popups", "allow-popups-to-escape-sandbox", "allow-top-navigation-by-user-activation", "allow-storage-access-by-user-activation"

You could just add the conditional inside the array declaration here. But as I argue earlier, I don't think it should be conditional at all.

We should check if the feature is turned on, and just ignore the policy if we don't have it active.

[John Wilander]

Created attachment 318954 [details]
Patch

[John Wilander]

(In reply to Andy Estes from comment #7)
> Comment on attachment 318613 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=318613&action=review
> 
> Not a full review, but I do have a few comments.
> 
> > Source/WebCore/dom/Document.cpp:4990
> > +    if (!Settings::storageAccessAPIEnabled())
> > +        return false;
> 
> This isn't a great way to disable this feature at runtime. In script, you
> want "window.requestStorageAccess" to be undefined with the feature
> disabled, but it won't be when implemented like this. The right way to do
> this is to annotate your methods with "EnabledBySetting" in the IDL file.

Addressed in new patch.

> > Source/WebCore/dom/Document.cpp:5012
> > +    if (!UserGestureIndicator::processingUserGesture())
> > +        return false;
> > +
> > +    if (isSandboxed(SandboxStorageAccessByUserActivation))
> > +        return false;
> 
> These checks seem too late. If I call this once from the main frame without
> a user gesture, then m_hasStorageAccess will be set to true but the function
> will return false. If I then call it a second time (still from the main
> frame without a user gesture), won't it then return true due to the "if
> (m_frame->isMainFrame())" check above?

I think what you spotted was a bug in the main frame case. It should both set the property and return true. See new patch. I also augmented the test cases to check both the return value and the property. https://bugs.webkit.org/show_bug.cgi?id=175919 is a follow-up for moving from a returned boolean to a promise.

> > Source/WebCore/dom/SecurityContext.cpp:89
> > +    static const char* const supportedPolicies[] = {
> > +        "allow-forms", "allow-same-origin", "allow-scripts", "allow-top-navigation", "allow-pointer-lock", "allow-popups", "allow-popups-to-escape-sandbox", "allow-top-navigation-by-user-activation", "allow-storage-access-by-user-activation"
> 
> Is it ok to define new sandbox flags that aren't specified by HTML? Is this
> flag being added to a spec?

I don't have a good answer here. I want the sandbox thing to be part of the experimental feature. Is there a way to make this conditional?

> > Source/WebCore/loader/ResourceLoadObserver.cpp:294
> > +    if (!m_storageAccessMap.contains(subFrameTopPrivatelyControlledDomain)) {
> > +        HashSet<String> topDomainEntry { topFrameTopPrivatelyControlledDomain };
> > +        m_storageAccessMap.set(subFrameTopPrivatelyControlledDomain, WTFMove(topDomainEntry));
> > +    } else if (m_storageAccessMap.get(subFrameTopPrivatelyControlledDomain).contains(topFrameTopPrivatelyControlledDomain))
> > +        return;
> 
> You should use HashMap::add() with an empty value, then check the AddResult.
> If "isNewEntry" is true, then you can use the iterator to set a new value.
> Otherwise, you can check the iterator to see if it contains
> topFrameTopPrivatelyControlledDomain.

Addressed in the new patch.

Thanks, Andy!

[John Wilander]

(In reply to Brent Fulgham from comment #11)
> Comment on attachment 318645 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=318645&action=review
> 
> This is a great start, but let's handle the experimental feature the right
> way from the start. r- to correct that. :-)
> 
> > Source/JavaScriptCore/Configurations/FeatureDefines.xcconfig:157
> > +ENABLE_STORAGE_ACCESS_API = ENABLE_STORAGE_ACCESS_API;
> 
> I don't think we want to add more "compile-time" guards like this for
> experimental features. We want to expose them as runtime-configurable
> switches we can control through the User Agent application (e.g., Safari's
> Developer menu). This doesn't seem like a feature that requires special
> back-end support or other code that needs to be compiled out for WebKit
> ports that are missing some critical platform feature. I suggest getting rid
> of this feature definition entirely.

Got it. Fixed in new patch.

> > Source/WebCore/dom/Document.cpp:5014
> > +    String returnValue;
> 
> This doesn't seem to be used.

Oops! Good catch. I wonder why I didn't get a compile error?

> > Source/WebCore/dom/Document.idl:125
> > +    [Conditional=STORAGE_ACCESS_API] boolean requestStorageAccess();
> 
> As Andy pointed out in his review, this should be
> "EnabledBySetting=StorageAccessAPI". Then update
> "WebKit/Shared/WebPreferencesDefinition.h" and add "StorageAccessAPI" to the
> FOR_EACH_WEBKIT_EXPERIMENTAL_FEATURE_PREFERENCE macro. Take a look at
> SubresourceIntegrity/SubresourceIntegrityEnabled to see the right way to do
> this.

Thanks. See new patch.

> > Source/WebCore/dom/SecurityContext.cpp:89
> > +        "allow-forms", "allow-same-origin", "allow-scripts", "allow-top-navigation", "allow-pointer-lock", "allow-popups", "allow-popups-to-escape-sandbox", "allow-top-navigation-by-user-activation", "allow-storage-access-by-user-activation"
> 
> You could just add the conditional inside the array declaration here. But as
> I argue earlier, I don't think it should be conditional at all.
> 
> We should check if the feature is turned on, and just ignore the policy if
> we don't have it active.

Is there a way for me to check it in the SecurityContext? I don't have the page's settings.

Thanks for the review, Brent!

[John Wilander]

Filed follow-up:
Storage Access API: Make Document::requestStorageAccess() async and return a promise
https://bugs.webkit.org/show_bug.cgi?id=175919

... based on comments from Tess O'Connor.

[John Wilander]

Created attachment 319029 [details]
Patch

[John Wilander]

Changed the behavior to only allow calls to this API from sandboxed iframes. This change was the result of a discussion with Brady Eidson and the reasoning is that the embedder should have to explicitly allow this.

[Chris Dumez]

Comment on attachment 319029 [details]
Patch

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

> Source/WebCore/dom/Document.cpp:4996
> +    if (m_frame->isMainFrame()) {

Nothing guarantees the document has a frame.

> Source/WebCore/dom/Document.cpp:5004
> +    SecurityOrigin& thisSecurityOrigin = securityOrigin();

I think we should just call this securityOrigin.

> Source/WebCore/dom/Document.cpp:5005
> +    SecurityOrigin& topSecurityOrigin = m_frame->tree().top().document()->securityOrigin();

topDocument().securityOrigin();

> Source/WebCore/dom/Document.cpp:5021
> +    if ((page && page->chrome().runJavaScriptConfirm(*m_frame, builder.toString())) || m_grantStorageAccessOverride) {

What happens if requestStorageAccess() is called by the JS in a loop?
Do we usually ask for permission from the user in this fashion? What about localization?

> Source/WebCore/dom/Document.cpp:7134
> +bool Document::isSandboxedIframe() const

I don't think this should be on document.

> Source/WebCore/dom/Document.idl:124
> +    [EnabledBySetting=StorageAccessAPI] readonly attribute boolean hasStorageAccess;

FYI, you are going to have to use EnabledAtRuntime instead of EnabledBySetting if you plan to expose things to workers.

Also, is this API specified somewhere? "hasStorageAccess" is super generic and does not really tell me what this is. Same comment for the method below.

> Source/WebCore/html/HTMLFrameOwnerElement.h:57
> +    bool isSandboxedIframe() const { return m_isSandboxedIframe; }

I am not convinced we need those on the frame owner element. Why isn't sandboxFlags() != SandboxNone sufficient? Also, it is pretty uncommon to not check for a specific sandbox flag.

> Source/WebCore/html/HTMLIFrameElement.cpp:96
> +        setIsSandboxedIframe(true);

This looks fishy.

[John Wilander]

Thanks, Chris! See below.

(In reply to Chris Dumez from comment #18)
> Comment on attachment 319029 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=319029&action=review
> 
> > Source/WebCore/dom/Document.cpp:4996
> > +    if (m_frame->isMainFrame()) {
> 
> Nothing guarantees the document has a frame.

Will fix.

> > Source/WebCore/dom/Document.cpp:5004
> > +    SecurityOrigin& thisSecurityOrigin = securityOrigin();
> 
> I think we should just call this securityOrigin.

Sure.

> > Source/WebCore/dom/Document.cpp:5005
> > +    SecurityOrigin& topSecurityOrigin = m_frame->tree().top().document()->securityOrigin();
> 
> topDocument().securityOrigin();

Got it.

> > Source/WebCore/dom/Document.cpp:5021
> > +    if ((page && page->chrome().runJavaScriptConfirm(*m_frame, builder.toString())) || m_grantStorageAccessOverride) {
> 
> What happens if requestStorageAccess() is called by the JS in a loop?
> Do we usually ask for permission from the user in this fashion? What about
> localization?

First, this is a place holder for some dedicated way of asking for the user's permission. We want to have something to demo at a standards meeting.

Second, I foresee this API being limited in how many times it can be called by a sub frame origin per, say, day.

Third, I will move to returning a promise as discussed above.

Forth, given its current proof-of-concept implementation it'll work as if the regular confirm() API is called in a loop, right?

Finally, localization will have to be figured out for sure. This is a proof-of-concept.

> > Source/WebCore/dom/Document.cpp:7134
> > +bool Document::isSandboxedIframe() const
> 
> I don't think this should be on document.

Any suggestion where it should be?

> > Source/WebCore/dom/Document.idl:124
> > +    [EnabledBySetting=StorageAccessAPI] readonly attribute boolean hasStorageAccess;
> 
> FYI, you are going to have to use EnabledAtRuntime instead of
> EnabledBySetting if you plan to expose things to workers.

I don't see any reason to expose this to workers as of now.

> Also, is this API specified somewhere? "hasStorageAccess" is super generic
> and does not really tell me what this is. Same comment for the method below.
> 
> > Source/WebCore/html/HTMLFrameOwnerElement.h:57
> > +    bool isSandboxedIframe() const { return m_isSandboxedIframe; }
> 
> I am not convinced we need those on the frame owner element. Why isn't
> sandboxFlags() != SandboxNone sufficient? Also, it is pretty uncommon to not
> check for a specific sandbox flag.

Here's my understanding:
SandboxNone means (a) there is a sandbox && (b) there are no openings to that sandbox.

sandboxFlags() != SandboxNone
No sandbox => false
A sandbox with no tokens => false
A sandbox with one or more tokens => true

I see what you mean that such a test would suffice for me but it doesn't answer the question "Is there a sandbox?" Is that the change you suggest? I wanted it to be very clear in the code that we require a sandbox and not only allow-storage-access-by-user-activation _if_ there is a sandbox.
   If we go with this, would you accept a code comment explaining the reasoning?

> > Source/WebCore/html/HTMLIFrameElement.cpp:96
> > +        setIsSandboxedIframe(true);
> 
> This looks fishy.

Why? (It may go away if we go with the sandboxFlags() != SandboxNone test.)

[Chris Dumez]

Comment on attachment 319029 [details]
Patch

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

>>> Source/WebCore/dom/Document.idl:124
>>> +    [EnabledBySetting=StorageAccessAPI] readonly attribute boolean hasStorageAccess;
>> 
>> FYI, you are going to have to use EnabledAtRuntime instead of EnabledBySetting if you plan to expose things to workers.
>> 
>> Also, is this API specified somewhere? "hasStorageAccess" is super generic and does not really tell me what this is. Same comment for the method below.
> 
> I don't see any reason to expose this to workers as of now.

How about my comment about the naming?

>>> Source/WebCore/html/HTMLFrameOwnerElement.h:57
>>> +    bool isSandboxedIframe() const { return m_isSandboxedIframe; }
>> 
>> I am not convinced we need those on the frame owner element. Why isn't sandboxFlags() != SandboxNone sufficient? Also, it is pretty uncommon to not check for a specific sandbox flag.
> 
> Here's my understanding:
> SandboxNone means (a) there is a sandbox && (b) there are no openings to that sandbox.
> 
> sandboxFlags() != SandboxNone
> No sandbox => false
> A sandbox with no tokens => false
> A sandbox with one or more tokens => true
> 
> I see what you mean that such a test would suffice for me but it doesn't answer the question "Is there a sandbox?" Is that the change you suggest? I wanted it to be very clear in the code that we require a sandbox and not only allow-storage-access-by-user-activation _if_ there is a sandbox.
>    If we go with this, would you accept a code comment explaining the reasoning?

I do not think your comment is correct.

If there is no sandbox in the HTML, then sandboxFlags() should return SandboxNone AFAIK. SandboxFlag is not about allowances but about things that are restricted. The native flags are the opposite of the HTML flags.

>>> Source/WebCore/html/HTMLIFrameElement.cpp:96
>>> +        setIsSandboxedIframe(true);
>> 
>> This looks fishy.
> 
> Why? (It may go away if we go with the sandboxFlags() != SandboxNone test.)

Nobody ever needed this. The fact that this new non-standard API requires it may indicate you are doing it wrong.

[John Wilander]

(In reply to Chris Dumez from comment #20)
> Comment on attachment 319029 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=319029&action=review
> 
> >>> Source/WebCore/dom/Document.idl:124
> >>> +    [EnabledBySetting=StorageAccessAPI] readonly attribute boolean hasStorageAccess;
> >> 
> >> FYI, you are going to have to use EnabledAtRuntime instead of EnabledBySetting if you plan to expose things to workers.
> >> 
> >> Also, is this API specified somewhere? "hasStorageAccess" is super generic and does not really tell me what this is. Same comment for the method below.
> > 
> > I don't see any reason to expose this to workers as of now.
> 
> How about my comment about the naming?

Sorry. I picked a generic name for now since we will bring this up with the standards WG anyway. It may be scoped to all (partitioned) storage or it may be restricted to cookies. I'm all ears if you have a better name that still sounds specification-worthy. :)

> >>> Source/WebCore/html/HTMLFrameOwnerElement.h:57
> >>> +    bool isSandboxedIframe() const { return m_isSandboxedIframe; }
> >> 
> >> I am not convinced we need those on the frame owner element. Why isn't sandboxFlags() != SandboxNone sufficient? Also, it is pretty uncommon to not check for a specific sandbox flag.
> > 
> > Here's my understanding:
> > SandboxNone means (a) there is a sandbox && (b) there are no openings to that sandbox.
> > 
> > sandboxFlags() != SandboxNone
> > No sandbox => false
> > A sandbox with no tokens => false
> > A sandbox with one or more tokens => true
> > 
> > I see what you mean that such a test would suffice for me but it doesn't answer the question "Is there a sandbox?" Is that the change you suggest? I wanted it to be very clear in the code that we require a sandbox and not only allow-storage-access-by-user-activation _if_ there is a sandbox.
> >    If we go with this, would you accept a code comment explaining the reasoning?
> 
> I do not think your comment is correct.
> 
> If there is no sandbox in the HTML, then sandboxFlags() should return
> SandboxNone AFAIK. SandboxFlag is not about allowances but about things that
> are restricted. The native flags are the opposite of the HTML flags.

OK, I'll test this.

> >>> Source/WebCore/html/HTMLIFrameElement.cpp:96
> >>> +        setIsSandboxedIframe(true);
> >> 
> >> This looks fishy.
> > 
> > Why? (It may go away if we go with the sandboxFlags() != SandboxNone test.)
> 
> Nobody ever needed this. The fact that this new non-standard API requires it
> may indicate you are doing it wrong.

It was the result of a conversation with Brady where he said we should have some kind of convenience function to ask if an iframe is sandboxed. He has not looked at my implementation though.

[John Wilander]

Created attachment 319098 [details]
Patch

[John Wilander]

Naming the local variable securityOrigin cases a compile time error because of the naming conflict with securityOrigin().

You were right that sandboxFlags() == SandboxNone works as test. This means that neither SandboxNone nor SandboxAll implies an existing sandbox with no allow tokens.

[Chris Dumez]

(In reply to John Wilander from comment #23)
> Naming the local variable securityOrigin cases a compile time error because
> of the naming conflict with securityOrigin().

Sure, which is why we usually do:
auto securityOrigin = this->securityOrigin()

> 
> You were right that sandboxFlags() == SandboxNone works as test. This means
> that neither SandboxNone nor SandboxAll implies an existing sandbox with no
> allow tokens.

[John Wilander]

Created attachment 319101 [details]
Patch

[John Wilander]

(In reply to Chris Dumez from comment #24)
> (In reply to John Wilander from comment #23)
> > Naming the local variable securityOrigin cases a compile time error because
> > of the naming conflict with securityOrigin().
> 
> Sure, which is why we usually do:
> auto securityOrigin = this->securityOrigin()

Aha. Will fix before landing. Please see new patch for the rest of the changes. Thx.

[John Wilander]

Requesting a review of the latest patch.

[Alex Christensen]

Comment on attachment 319101 [details]
Patch

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

> Source/WebCore/dom/Document.cpp:5027
> +    builder.appendLiteral("Do you want to use your ");

This should definitely eventually become a WEB_UI_STRING so it can be localized.

> Source/WebCore/dom/Document.cpp:5033
> +    if ((page && page->chrome().runJavaScriptConfirm(*m_frame, builder.toString())) || m_grantStorageAccessOverride) {

I think you'll find that when making requestStorageAccess return a promise you'll need to use a new Chrome/ChromeClient function that takes a Function<void(bool)>&& as a parameter (or the new CompletionHandler template) and that will make the flow of this patch and the tests significantly different.  I think landing this patch first will make that patch significantly harder to write and get working with these tests, but that is the direction we must go.

[John Wilander]

Created attachment 320579 [details]
Patch

[John Wilander]

Thanks for the review comments, Alex! The new patch uses a promise in stead of a synchronous return.

[John Wilander]

Created attachment 320580 [details]
Patch

[John Wilander]

Fixed IDL mistake.

[Alex Christensen]

Comment on attachment 320580 [details]
Patch

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

> Source/WebCore/dom/Document.cpp:7337
> +    if ((page && page->chrome().runJavaScriptConfirm(*m_frame, builder.toString())) || m_grantStorageAccessOverride) {

Please add a FIXME: Don't use runJavaScriptConfirm because it responds synchronously.

[John Wilander]

Created attachment 320651 [details]
Patch for landing

[John Wilander]

(In reply to Alex Christensen from comment #33)
> Comment on attachment 320580 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=320580&action=review
> 
> > Source/WebCore/dom/Document.cpp:7337
> > +    if ((page && page->chrome().runJavaScriptConfirm(*m_frame, builder.toString())) || m_grantStorageAccessOverride) {
> 
> Please add a FIXME: Don't use runJavaScriptConfirm because it responds
> synchronously.

Will do. Thanks for the review, Alex!

[WebKit Commit Bot]

Comment on attachment 320651 [details]
Patch for landing

Clearing flags on attachment: 320651

Committed r221976: <http://trac.webkit.org/changeset/221976>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[John Wilander]

Actually using rdar://problem/34414107 to track this particular piece and rdar://problem/33666847 to track the overall work.

Will mark https://bugs.webkit.org/show_bug.cgi?id=175919 as resolved since the promise work ended up here after all.

[John Wilander]

*** Bug 175919 has been marked as a duplicate of this bug. ***

[Matt Lewis]

Reverted r221976 for reason:

The test introduced was flaky from point of addition.

Committed r221997: <http://trac.webkit.org/changeset/221997>

[Matt Lewis]

History:

https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Ftests%2Floading%2FresourceLoadStatistics%2Frequest-and-grant-storage-access-cross-origin-iframe.html

Failing Results:

https://build.webkit.org/results/Apple%20Sierra%20Release%20WK2%20(Tests)/r221990%20(4266)/results.html

Diff:

--- /Volumes/Data/slave/sierra-release-tests-wk2/build/layout-test-results/http/tests/loading/resourceLoadStatistics/request-and-grant-storage-access-cross-origin-iframe-expected.txt
+++ /Volumes/Data/slave/sierra-release-tests-wk2/build/layout-test-results/http/tests/loading/resourceLoadStatistics/request-and-grant-storage-access-cross-origin-iframe-actual.txt
@@ -1,13 +1,13 @@
 main frame - didStartProvisionalLoadForFrame
 main frame - didCommitLoadForFrame
 frame "theIframe" - didStartProvisionalLoadForFrame
-main frame - didFinishDocumentLoadForFrame
 frame "theIframe" - didCommitLoadForFrame
 frame "theIframe" - didFinishDocumentLoadForFrame
 CONSOLE MESSAGE: line 70: PASS document.hasStorageAccess was denied.
 frame "theIframe" - didHandleOnloadEventsForFrame
+frame "theIframe" - didFinishLoadForFrame
+main frame - didFinishDocumentLoadForFrame
 main frame - didHandleOnloadEventsForFrame
-frame "theIframe" - didFinishLoadForFrame
 main frame - didFinishLoadForFrame
 Tests that cross-origin iframe storage access is denied if the iframe is not sandboxed.

[John Wilander]

Created attachment 320721 [details]
Patch

[John Wilander]

Comment on attachment 320721 [details]
Patch

Moved the tests to where they don't cause frame loading output in the expect files. Waiting to get green light on EWS before landing.

[WebKit Commit Bot]

Comment on attachment 320721 [details]
Patch

Clearing flags on attachment: 320721

Committed r222006: <http://trac.webkit.org/changeset/222006>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.