Bug 180659 – Layout Test http/tests/workers/service/postmessage-after-sw-process-crash.https.html is flaky

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Ryan Haddad]

The following layout test is flaky on macOS and iOS WK2:

http/tests/workers/service/postmessage-after-sw-process-crash.https.html

Probable cause:

Test was added with https://trac.webkit.org/changeset/225622/webkit

Flakiness Dashboard:

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

--- /Volumes/Data/slave/elcapitan-release-tests-wk2/build/layout-test-results/http/tests/workers/service/postmessage-after-sw-process-crash.https-expected.txt
+++ /Volumes/Data/slave/elcapitan-release-tests-wk2/build/layout-test-results/http/tests/workers/service/postmessage-after-sw-process-crash.https-actual.txt
@@ -3,6 +3,5 @@
 PASS: Service worker received message 'Message 1' from origin 'https://127.0.0.1:8443'
 * Simulating Service Worker process crash
 * Sending 'Message 2' to Service Worker
-PASS: Client received message from service worker, origin: https://127.0.0.1:8443
-PASS: Service worker received message 'Message 2' from origin 'https://127.0.0.1:8443'
+FAIL: Did not receive message from service worker process after the crash

[Chris Dumez]

Will take a look.

[Chris Dumez]

Created attachment 329027 [details]
Patch

[youenn fablet]

Comment on attachment 329027 [details]
Patch

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

> Source/WebKit/StorageProcess/ServiceWorker/WebSWServerConnection.cpp:133
> +    server().runServiceWorkerIfNecessary(destinationIdentifier, [destinationIdentifier, message = message.vector(), sourceIdentifier, sourceData = WTFMove(sourceData)](bool success, auto& contextConnection) mutable {

No need for message to be passed as an r value.

This is so easy to fall into the trap of copying a DataReference and thinking that it will copy the data it refers to.
Maybe DataReference should not be copy constructible, one can still explicitly create a DataReference with one of its other constructor.

> LayoutTests/http/tests/workers/service/resources/postmessage-after-sw-process-crash.js:18
> +        }, 10);

For that kind of pattern, I usually like to do this ping for a number of time and fail afterwards with an explicit message.
This allows making test messages in case of timeouts more precise.

[Chris Dumez]

Created attachment 329034 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 329034 [details]
Patch

Clearing flags on attachment: 329034

Committed r225758: <https://trac.webkit.org/changeset/225758>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/35979040>