Bug 181601 – Layout Test http/tests/storageAccess/request-and-grant-access-then-detach-should-not-have-access.html is flaky

• Component: New Bugs
• Status: RESOLVED FIXED

[Ryan Haddad]

The following layout test is flaky on High Sierra Release WK2

http/tests/storageAccess/request-and-grant-access-then-detach-should-not-have-access.html

Probable cause:

This test appears to have been flaky since it was added in https://trac.webkit.org/r226235

Flakiness Dashboard:

https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Ftests%2FstorageAccess%2Frequest-and-grant-access-then-detach-should-not-have-access.html

--- /Volumes/Data/slave/highsierra-release-tests-wk2/build/layout-test-results/http/tests/storageAccess/request-and-grant-access-then-detach-should-not-have-access-expected.txt
+++ /Volumes/Data/slave/highsierra-release-tests-wk2/build/layout-test-results/http/tests/storageAccess/request-and-grant-access-then-detach-should-not-have-access-actual.txt
@@ -3,9 +3,4 @@
 On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
 
 
-PASS Storage access was granted. document.cookie == firstPartyCookie=value, cookies seen server-side == {"firstPartyCookie":"value"}
-PASS PASS. document.cookie == , cookies seen server-side == "No cookies"
-PASS successfullyParsed is true
 
-TEST COMPLETE
-

[Radar WebKit Bug Importer]

<rdar://problem/36475837>

[Matt Lewis]

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

[Matt Lewis]

Marked as flaky: https://trac.webkit.org/changeset/227574/webkit

[John Wilander]

Created attachment 332832 [details]
Patch

[Alex Christensen]

Comment on attachment 332832 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStorePrivate.h:91
> +- (void)_hasStorageAccessEntry:(NSString *)host completionHandler:(void (^)(BOOL))completionHandler WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA));

Give this a WKWebView* from which you get the pageID instead of host.  Have the return value be an NSDictionary<_WKFrameHandle*, NSString*> to get the storage access entries for this WKWebView.

> Tools/WebKitTestRunner/InjectedBundle/Bindings/TestRunner.idl:180
> +    boolean hasStorageAccessEntry(DOMString hostName);

What if we had this return a promise so we don't need to add more synchronous stuff?
I think the promise should give you an array of partitions instead of just a bool.

[Chris Dumez]

(In reply to Alex Christensen from comment #5)
> Comment on attachment 332832 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=332832&action=review
> 
> > Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStorePrivate.h:91
> > +- (void)_hasStorageAccessEntry:(NSString *)host completionHandler:(void (^)(BOOL))completionHandler WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA));
> 
> Give this a WKWebView* from which you get the pageID instead of host.  Have
> the return value be an NSDictionary<_WKFrameHandle*, NSString*> to get the
> storage access entries for this WKWebView.
> 
> > Tools/WebKitTestRunner/InjectedBundle/Bindings/TestRunner.idl:180
> > +    boolean hasStorageAccessEntry(DOMString hostName);
> 
> What if we had this return a promise so we don't need to add more
> synchronous stuff?
> I think the promise should give you an array of partitions instead of just a
> bool.

Note that I do not believe we support promises in TestRunner IDL (which uses its own bindings generator). We do support callbacks though.

[John Wilander]

Created attachment 333024 [details]
Patch

[Alex Christensen]

Comment on attachment 333024 [details]
Patch

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

> Tools/WebKitTestRunner/TestController.cpp:2674
> +//    auto* dataStore = WKContextGetWebsiteDataStore(platformContext());

Please don't land this commented-out code.

[John Wilander]

Created attachment 333098 [details]
Patch for landing

[John Wilander]

(In reply to Alex Christensen from comment #8)
> Comment on attachment 333024 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=333024&action=review
> 
> > Tools/WebKitTestRunner/TestController.cpp:2674
> > +//    auto* dataStore = WKContextGetWebsiteDataStore(platformContext());
> 
> Please don't land this commented-out code.

It was intended as a friendly gesture to the Linux maintainers. Will remove.

Thanks for the review!

[WebKit Commit Bot]

Comment on attachment 333098 [details]
Patch for landing

Clearing flags on attachment: 333098

Committed r228109: <https://trac.webkit.org/changeset/228109>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Matt Lewis]

The commit doesn't seem to have fixed the flakiness of the test:

https://build.webkit.org/results/Apple%20High%20Sierra%20Release%20WK2%20(Tests)/r228129%20(2753)/results.html
https://build.webkit.org/builders/Apple%20High%20Sierra%20Release%20WK2%20(Tests)/builds/2753

[John Wilander]

I will add additional logging to the test output to see if we can understand why the bots see these failures.

[Shawn Roberts]

Test is still flaky, also being flaky on Mojave WK2 and Debug.

https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Ftests%2FstorageAccess%2Frequest-and-grant-access-then-detach-should-not-have-access.html