Bug 181601 - Layout Test http/tests/storageAccess/request-and-grant-access-then-detach-should-not-have-access.html is flaky
Summary: Layout Test http/tests/storageAccess/request-and-grant-access-then-detach-sho...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: John Wilander
URL:
Keywords: InRadar
: 181605 (view as bug list)
Depends on:
Blocks:
 
Reported: 2018-01-12 10:59 PST by Ryan Haddad
Modified: 2019-02-01 11:34 PST (History)
9 users (show)

See Also:


Attachments
Patch (38.31 KB, patch)
2018-01-31 18:39 PST, John Wilander
no flags Details | Formatted Diff | Diff
Patch (37.46 KB, patch)
2018-02-02 18:11 PST, John Wilander
no flags Details | Formatted Diff | Diff
Patch for landing (37.10 KB, patch)
2018-02-05 11:01 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 Ryan Haddad 2018-01-12 10:59:53 PST
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
-
Comment 1 Radar WebKit Bug Importer 2018-01-12 11:01:05 PST
<rdar://problem/36475837>
Comment 2 Matt Lewis 2018-01-24 14:32:26 PST
*** Bug 181605 has been marked as a duplicate of this bug. ***
Comment 3 Matt Lewis 2018-01-24 14:39:47 PST
Marked as flaky: https://trac.webkit.org/changeset/227574/webkit
Comment 4 John Wilander 2018-01-31 18:39:40 PST
Created attachment 332832 [details]
Patch
Comment 5 Alex Christensen 2018-01-31 19:28:01 PST
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.
Comment 6 Chris Dumez 2018-01-31 19:52:45 PST
(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.
Comment 7 John Wilander 2018-02-02 18:11:24 PST
Created attachment 333024 [details]
Patch
Comment 8 Alex Christensen 2018-02-05 10:56:15 PST
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.
Comment 9 John Wilander 2018-02-05 11:01:37 PST
Created attachment 333098 [details]
Patch for landing
Comment 10 John Wilander 2018-02-05 11:03:06 PST
(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!
Comment 11 WebKit Commit Bot 2018-02-05 11:35:18 PST
Comment on attachment 333098 [details]
Patch for landing

Clearing flags on attachment: 333098

Committed r228109: <https://trac.webkit.org/changeset/228109>
Comment 12 WebKit Commit Bot 2018-02-05 11:35:20 PST
All reviewed patches have been landed.  Closing bug.
Comment 14 John Wilander 2018-02-05 17:14:20 PST
I will add additional logging to the test output to see if we can understand why the bots see these failures.