Layout Test imported/w3c/web-platform-tests/service-workers/service-worker/register-same-scope-different-script-url.https.html is flaky

[Matt Lewis]

The following layout test is flaky on WK2

imported/w3c/web-platform-tests/service-workers/service-worker/register-same-scope-different-script-url.https.html

Probable cause:

https://trac.webkit.org/changeset/226274/webkit

Flakiness Dashboard:

https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=imported%2Fw3c%2Fweb-platform-tests%2Fservice-workers%2Fservice-worker%2Fregister-same-scope-different-script-url.https.html


This test recently became extremely flaky. 

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

--- /Volumes/Data/slave/highsierra-release-tests-wk2/build/layout-test-results/imported/w3c/web-platform-tests/service-workers/service-worker/register-same-scope-different-script-url.https-expected.txt
+++ /Volumes/Data/slave/highsierra-release-tests-wk2/build/layout-test-results/imported/w3c/web-platform-tests/service-workers/service-worker/register-same-scope-different-script-url.https-actual.txt
@@ -1,7 +1,9 @@
+
+Harness Error (TIMEOUT), message = null
 
 PASS Register different scripts concurrently 
 PASS Register then register new script URL 
 PASS Register then register new script URL that 404s 
 FAIL Register then register new script that does not install assert_unreached: unexpected rejection: assert_equals: on redundant, installing should be null expected null but got object "[object ServiceWorker]" Reached unreachable code
-PASS Register same-scope new script url effect on controller 
+TIMEOUT Register same-scope new script url effect on controller Test timed out

[Matt Lewis]

Marked as flaky in:
https://trac.webkit.org/changeset/226296/webkit

[Chris Dumez]

I managed to reproduce the flakiness like so:
run-webkit-tests -gf --repeat-each=25 imported/w3c/web-platform-tests/service-workers/service-worker/register-same-scope-different-script-url.https.html

[Radar WebKit Bug Importer]

<rdar://problem/37169508>

[Chris Dumez]

Looks like the registration.unregister() promise at the end of the last test sometimes does not get resolved.

[Chris Dumez]

If I disable soft-update, then it is no longer flaky. It seems that SoftUpdate jobs sometimes "hang" the jobQueue so that it is no longer processing new jobs.

[Chris Dumez]

Created attachment 333054 [details]
Patch

[youenn fablet]

Comment on attachment 333054 [details]
Patch

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

> Source/WebCore/workers/service/ServiceWorkerJobData.cpp:39
>      : m_identifier { connectionIdentifier, generateThreadSafeObjectIdentifier<ServiceWorkerJobIdentifierType>() }

Preexisting issue but seems a bit weird to have m_identifier here and sourceContext which are both member fields of ServiceWorkerJobData.

> Source/WebCore/workers/service/server/SWServer.cpp:503
> +        // Abort if the job that scheduled this has not been cancelled.

s/has not/has/

> Source/WebCore/workers/service/server/SWServer.cpp:633
> +        jobQueue->cancelJobsFromServiceWorker(worker.identifier());

Maybe just me but I would read it more easily with something like cancelServiceWorkerJobs. Ditto below.

[Chris Dumez]

(In reply to youenn fablet from comment #7)
> Comment on attachment 333054 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=333054&action=review
> 
> > Source/WebCore/workers/service/ServiceWorkerJobData.cpp:39
> >      : m_identifier { connectionIdentifier, generateThreadSafeObjectIdentifier<ServiceWorkerJobIdentifierType>() }
> 
> Preexisting issue but seems a bit weird to have m_identifier here and
> sourceContext which are both member fields of ServiceWorkerJobData.
> 
> > Source/WebCore/workers/service/server/SWServer.cpp:503
> > +        // Abort if the job that scheduled this has not been cancelled.
> 
> s/has not/has/
> 
> > Source/WebCore/workers/service/server/SWServer.cpp:633
> > +        jobQueue->cancelJobsFromServiceWorker(worker.identifier());
> 
> Maybe just me but I would read it more easily with something like
> cancelServiceWorkerJobs. Ditto below.

I personally prefer cancelJobsFromServiceWorker to cancelServiceWorkerJobs. "cancelServiceWorkerJobs" sounds very generic given that we are in service worker code.

[Chris Dumez]

Created attachment 333086 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 333086 [details]
Patch

Clearing flags on attachment: 333086

Committed r228101: <https://trac.webkit.org/changeset/228101>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.