Bug 183705 – [ WK2 ] http/tests/workers/service/client-*-page-cache.html LayoutTests are flaky

• Status: RESOLVED
• Resolution: FIXED
• Component: Service Workers

[Ryan Haddad]

The following two LayoutTests are flaky failures on iOS and macOS WK2:

http/tests/workers/service/client-added-to-clients-when-restored-from-page-cache.html	
http/tests/workers/service/client-removed-from-clients-while-in-page-cache.html

https://build.webkit.org/results/Apple%20Sierra%20Debug%20WK2%20(Tests)/r229663%20(5649)/results.html

https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Ftests%2Fworkers%2Fservice%2Fclient


Both tests fail with a similar diff:

--- /Volumes/Data/slave/sierra-debug-tests-wk2/build/layout-test-results/http/tests/workers/service/client-added-to-clients-when-restored-from-page-cache-expected.txt
+++ /Volumes/Data/slave/sierra-debug-tests-wk2/build/layout-test-results/http/tests/workers/service/client-added-to-clients-when-restored-from-page-cache-actual.txt
@@ -1,6 +1,4 @@
 * Tests that a client is re-added to the list of service worker clients when it is restored from the page cache
 
-PASS: service worker has initially 2 clients
-PASS: page is about to enter page cache
-PASS: service worker now has 2 clients again after restoring the second one from page cache
+FAIL: Wrong initial number of clients: 3

[Truitt Savell]

These two tests currently became extremely flaky along with a third. 

http/tests/workers/service/client-added-to-clients-when-restored-from-page-cache.html
http/tests/workers/service/client-removed-from-clients-while-in-page-cache.html

http/tests/workers/service/serviceworkerclients-matchAll.https.html

Combined History:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Ftests%2Fworkers%2Fservice%2Fserviceworkerclients-matchAll.https.html%20%20http%2Ftests%2Fworkers%2Fservice%2Fclient-added-to-clients-when-restored-from-page-cache.html%20%20http%2Ftests%2Fworkers%2Fservice%2Fclient-removed-from-clients-while-in-page-cache.html

They all began failing around r233895. 

serviceworkerclients-matchAll.https.html Text Diff:

--- /Volumes/Data/slave/highsierra-release-tests-wk2/build/layout-test-results/http/tests/workers/service/serviceworkerclients-matchAll.https-expected.txt
+++ /Volumes/Data/slave/highsierra-release-tests-wk2/build/layout-test-results/http/tests/workers/service/serviceworkerclients-matchAll.https-actual.txt
@@ -1,4 +1,4 @@
 
 PASS Setup worker 
-PASS Test self.clients.matchAll 
+FAIL Test self.clients.matchAll assert_equals: expected "PASS" but got "FAIL: expected one matched client, got 2"

These issues seem to be due to the test receiving one additional client than they expected.

[Truitt Savell]

Issue seems to have began between r233891 -- 233897. 

r233891 has specific changes to Clients: https://trac.webkit.org/changeset/233891/webkit

[Wenson Hsieh]

(In reply to Truitt Savell from comment #2)
> Issue seems to have began between r233891 -- 233897. 
> 
> r233891 has specific changes to Clients:
> https://trac.webkit.org/changeset/233891/webkit

I really doubt this is the cause. This patch has no behavior change unless clients opt into the new SPI, which WebKitTestRunner does not.

[Chris Dumez]

(In reply to Wenson Hsieh from comment #3)
> (In reply to Truitt Savell from comment #2)
> > Issue seems to have began between r233891 -- 233897. 
> > 
> > r233891 has specific changes to Clients:
> > https://trac.webkit.org/changeset/233891/webkit
> 
> I really doubt this is the cause. This patch has no behavior change unless
> clients opt into the new SPI, which WebKitTestRunner does not.

I am working on a bad crasher right now but I am happy to look into this bug right after that.

[Truitt Savell]

I noticed that 
http/tests/workers/service/client-added-to-clients-when-restored-from-page-cache.html
http/tests/workers/service/client-removed-from-clients-while-in-page-cache.html

always fail together. They may be getting stuck in a state together. Or because they getting more clients than they expect maybe something is providing too many clients earlier in the run.

[Chris Dumez]

Ok, I have some spare cycles to look into this.

[Chris Dumez]

> Issue seems to have began between r233891 -- 233897. 

Likely a regression from process-swap-on-navigation then, which was r233897.

[Chris Dumez]

(In reply to Chris Dumez from comment #7)
> > Issue seems to have began between r233891 -- 233897. 
> 
> Likely a regression from process-swap-on-navigation then, which was r233897.

Unable to reproduce so far:
Tools/Scripts/run-webkit-tests http/tests/workers/service/client-added-to-clients-when-restored-from-page-cache.html http/tests/workers/service/client-removed-from-clients-while-in-page-cache.html --repeat-each=100 -f

Also tried running the whole folder.

[Chris Dumez]

Created attachment 345463 [details]
Patch

[youenn fablet]

Comment on attachment 345463 [details]
Patch

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

> LayoutTests/http/tests/workers/service/client-added-to-clients-when-restored-from-page-cache.html:17
> +      worker.postMessage("getClientCount");

Shouldn't we do the usual pattern:
if (++counter < 20) {
  log("FAIL...")
  finishSWTest();
}
await waitFor(50);
worker.postMessage("getClientCount");

> LayoutTests/http/tests/workers/service/serviceworkerclients-matchAll-worker.js:26
> +            result = await matchAllPromise1;

This loop does not seem useful as is.
We might want to change matchAllPromise1 to a function which would return a new promise every time it is called.
Again a waitFor(50) might help.

[Chris Dumez]

Created attachment 345470 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 345470 [details]
Patch

Clearing flags on attachment: 345470

Committed r234061: <https://trac.webkit.org/changeset/234061>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/42440606>

[Truitt Savell]

(In reply to WebKit Commit Bot from comment #12)
> Comment on attachment 345470 [details]
> Patch
> 
> Clearing flags on attachment: 345470
> 
> Committed r234061: <https://trac.webkit.org/changeset/234061>

It does not look like this change made a difference. Test is still flakey and behavior seems to be the same. failure message is also the same.

[Truitt Savell]

reopening for visibility

[Chris Dumez]

(In reply to Truitt Savell from comment #15)
> (In reply to WebKit Commit Bot from comment #12)
> > Comment on attachment 345470 [details]
> > Patch
> > 
> > Clearing flags on attachment: 345470
> > 
> > Committed r234061: <https://trac.webkit.org/changeset/234061>
> 
> It does not look like this change made a difference. Test is still flakey
> and behavior seems to be the same. failure message is also the same.

Which test? What is the output?

[Chris Dumez]

(In reply to Chris Dumez from comment #17)
> (In reply to Truitt Savell from comment #15)
> > (In reply to WebKit Commit Bot from comment #12)
> > > Comment on attachment 345470 [details]
> > > Patch
> > > 
> > > Clearing flags on attachment: 345470
> > > 
> > > Committed r234061: <https://trac.webkit.org/changeset/234061>
> > 
> > It does not look like this change made a difference. Test is still flakey
> > and behavior seems to be the same. failure message is also the same.
> 
> Which test? What is the output?

The 2 page cache tests:
--- /Volumes/Data/slave/sierra-release-tests-wk2/build/layout-test-results/http/tests/workers/service/client-removed-from-clients-while-in-page-cache-expected.txt
+++ /Volumes/Data/slave/sierra-release-tests-wk2/build/layout-test-results/http/tests/workers/service/client-removed-from-clients-while-in-page-cache-actual.txt
@@ -1,6 +1,4 @@
 * Tests that a client is removed from the list of service worker clients while it is in the page cache
 
-PASS: service worker has initially 2 clients
-PASS: page is about to enter page cache
-PASS: service worker has only 1 client after 1 entered page cache
+FAIL: Wrong initial number of clients: 3

[Truitt Savell]

(In reply to Chris Dumez from comment #17)
> (In reply to Truitt Savell from comment #15)
> > (In reply to WebKit Commit Bot from comment #12)
> > > Comment on attachment 345470 [details]
> > > Patch
> > > 
> > > Clearing flags on attachment: 345470
> > > 
> > > Committed r234061: <https://trac.webkit.org/changeset/234061>
> > 
> > It does not look like this change made a difference. Test is still flakey
> > and behavior seems to be the same. failure message is also the same.
> 
> Which test? What is the output?

Sorry I meant tests. All three tests:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Ftests%2Fworkers%2Fservice%2Fserviceworkerclients-matchAll.https.html%20%20http%2Ftests%2Fworkers%2Fservice%2Fclient-added-to-clients-when-restored-from-page-cache.html%20%20http%2Ftests%2Fworkers%2Fservice%2Fclient-removed-from-clients-while-in-page-cache.html

are still having flakey failures.

Test Outputs:
https://build.webkit.org/results/Apple%20iOS%2011%20Simulator%20Debug%20WK2%20(Tests)/r234067%20(5511)/http/tests/workers/service/serviceworkerclients-matchAll.https-pretty-diff.html

https://build.webkit.org/results/Apple%20Sierra%20Release%20WK2%20(Tests)/r234064%20(10678)/http/tests/workers/service/client-added-to-clients-when-restored-from-page-cache-pretty-diff.html

[Chris Dumez]

(In reply to Truitt Savell from comment #19)
> (In reply to Chris Dumez from comment #17)
> > (In reply to Truitt Savell from comment #15)
> > > (In reply to WebKit Commit Bot from comment #12)
> > > > Comment on attachment 345470 [details]
> > > > Patch
> > > > 
> > > > Clearing flags on attachment: 345470
> > > > 
> > > > Committed r234061: <https://trac.webkit.org/changeset/234061>
> > > 
> > > It does not look like this change made a difference. Test is still flakey
> > > and behavior seems to be the same. failure message is also the same.
> > 
> > Which test? What is the output?
> 
> Sorry I meant tests. All three tests:
> https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.
> html#showAllRuns=true&tests=http%2Ftests%2Fworkers%2Fservice%2Fserviceworkerc
> lients-matchAll.https.html%20%20http%2Ftests%2Fworkers%2Fservice%2Fclient-
> added-to-clients-when-restored-from-page-cache.
> html%20%20http%2Ftests%2Fworkers%2Fservice%2Fclient-removed-from-clients-
> while-in-page-cache.html
> 
> are still having flakey failures.
> 
> Test Outputs:
> https://build.webkit.org/results/
> Apple%20iOS%2011%20Simulator%20Debug%20WK2%20(Tests)/r234067%20(5511)/http/
> tests/workers/service/serviceworkerclients-matchAll.https-pretty-diff.html
> 
> https://build.webkit.org/results/Apple%20Sierra%20Release%20WK2%20(Tests)/
> r234064%20(10678)/http/tests/workers/service/client-added-to-clients-when-
> restored-from-page-cache-pretty-diff.html

So maybe the clients are actually leaking in some cases? :/

[Ryan Haddad]

Marked tests as flaky in https://trac.webkit.org/r234100

[Chris Dumez]

Committed r234107: <https://trac.webkit.org/changeset/234107>

[Chris Dumez]

Reopening as this latest fix was only for http/tests/workers/service/serviceworkerclients-matchAll.https.html. It is works, I'll apply the same fix to the other 2 tests.

[Ryan Haddad]

(In reply to Chris Dumez from comment #23)
> Reopening as this latest fix was only for
> http/tests/workers/service/serviceworkerclients-matchAll.https.html. It is
> works, I'll apply the same fix to the other 2 tests.
Here is a failure on iOS Simulator after r234107:
https://build.webkit.org/results/Apple%20iOS%2011%20Simulator%20Release%20WK2%20(Tests)/r234109%20(6374)/results.html

[Ryan Haddad]

Marked http/tests/workers/service/serviceworkerclients-matchAll.https.html as flaky again in https://trac.webkit.org/r234170.

[Chris Dumez]

(In reply to Ryan Haddad from comment #25)
> Marked http/tests/workers/service/serviceworkerclients-matchAll.https.html
> as flaky again in https://trac.webkit.org/r234170.

Ok thanks. I will look into this again when I am done with more urgent matters.

[Chris Dumez]

Created attachment 348671 [details]
Patch

[youenn fablet]

Comment on attachment 348671 [details]
Patch

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

> Source/WebCore/testing/Internals.cpp:2331
> +String Internals::serviceWorkerClientIdentifier(Document& document) const

const Document& maybe?

> LayoutTests/ChangeLog:10
> +        identifiers to see if they are present or not.

Were these tests testable in a regular browser before?
Would it make sense to use internals API if available and rely on client count otherwise?

Looking further for some tests, for controlled/uncontrolled clients, it makes sense to use identifiers.

[Chris Dumez]

Comment on attachment 348671 [details]
Patch

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

>> Source/WebCore/testing/Internals.cpp:2331
>> +String Internals::serviceWorkerClientIdentifier(Document& document) const
> 
> const Document& maybe?

Canno

[Chris Dumez]

Comment on attachment 348671 [details]
Patch

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

>>> Source/WebCore/testing/Internals.cpp:2331
>>> +String Internals::serviceWorkerClientIdentifier(Document& document) const
>> 
>> const Document& maybe?
> 
> Canno

Oh, my previous implementation needed this non-const but the current implementation seems like it would allow it. Will check.

[Chris Dumez]

Created attachment 348674 [details]
Patch

[Chris Dumez]

Comment on attachment 348674 [details]
Patch

Clearing flags on attachment: 348674

Committed r235570: <https://trac.webkit.org/changeset/235570>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.