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.
<rdar://problem/33666847>
Created attachment 318613 [details] Patch
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
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
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
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
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.
Created attachment 318645 [details] Patch
Thanks, Andy! I uploaded a new patch to address the test failures but I'll look into your comments now.
(In reply to Andy Estes from comment #7) > you want "window.requestStorageAccess" to be undefined with the feature disabled "document.requestStorageAccess", that is.
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.
Created attachment 318954 [details] Patch
(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!
(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!
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.
Created attachment 319029 [details] Patch
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.
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.
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.)
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.
(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.
Created attachment 319098 [details] Patch
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.
(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.
Created attachment 319101 [details] Patch
(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.
Requesting a review of the latest patch.
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.
Created attachment 320579 [details] Patch
Thanks for the review comments, Alex! The new patch uses a promise in stead of a synchronous return.
Created attachment 320580 [details] Patch
Fixed IDL mistake.
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.
Created attachment 320651 [details] Patch for landing
(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!
Comment on attachment 320651 [details] Patch for landing Clearing flags on attachment: 320651 Committed r221976: <http://trac.webkit.org/changeset/221976>
All reviewed patches have been landed. Closing bug.
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.
*** Bug 175919 has been marked as a duplicate of this bug. ***
Reverted r221976 for reason: The test introduced was flaky from point of addition. Committed r221997: <http://trac.webkit.org/changeset/221997>
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.
Created attachment 320721 [details] Patch
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.
Comment on attachment 320721 [details] Patch Clearing flags on attachment: 320721 Committed r222006: <http://trac.webkit.org/changeset/222006>