[service-workers] Avoid race condition in test #4879
Conversation
The parent document in this test first sends a message to the ServiceWorker and later triggers a `fetch` event. Prior to this commit, the test initiated these two operations independently but required that the Service Worker's `onmessage` handler was invoked prior to its `onfetch` handler. However, because user agents may define independent task queues for `fetch` and `postMessage` [1], there the sequence in which the tasks are handled is not guaranteed. By incorrectly assuming the determinacy of event order, the test created a race condition in user agents that maintain two queues. Re-factor the test to force a consistent execution order. [1] From https://html.spec.whatwg.org/multipage/webappapis.html#task-queue > Each task is defined as coming from a specific task source. All the > tasks from one particular task source and destined to a particular > event loop (e.g. the callbacks generated by timers of a Document, the > events fired for mouse movements over that Document, the tasks queued > for the parser of that Document) must always be added to the same task > queue, but tasks from different task sources may be placed in > different task queues.
|
Notifying @ehsan and @mkruisselbrink. (Learn how reviewing works.) |
| with_iframe(scope).then(function(f) { frames.push(f); }); | ||
| } | ||
|
|
||
| function onMessage(event) { | ||
| function onResult(event) { |
There was a problem hiding this comment.
unrelated to this change, but at some level the call to onResult needs to be wrapped in t.step/t.step_func to make sure the asserts get attributed to the correct test (pretty much all the other async code also needs to be wrapped to properly attribute exceptions of course). Or alternatively since the entire file only has one async test anyway, we could just get rid of the async_test completely turning this into a single page test (but some of the helper methods like service_worker_unregister_and_done wouldn't work anymore then)
There was a problem hiding this comment.
Happy to address this in a follow up commit
| @@ -4,6 +4,8 @@ onmessage = function(e) { | |||
| var message = e.data; | |||
| if (typeof message === 'object' && 'port' in message) { | |||
| port = message.port; | |||
|
|
|||
| port.postMessage('received port'); | |||
| } | |||
| }; | |||
There was a problem hiding this comment.
nit: technically this test is still flaky, as nothing is stopping a browser from killing the service worker as soon as the message event is handled, and restarting it before the fetch event is dispatch. To fix that you should call waitUntil on the message event with a promise you either don't resolve, or at least don't resolve until the fetch event handler.
There was a problem hiding this comment.
I've pushed up a commit to address this. I'm using a custom "pending thenable" for this purpose in order to limit the number of features in this test. If you'd prefer to use a true Promise instance, say the word and I'll be happy to do so.
Chrome (unstable channel)Testing web-platform-tests at revision c076f25 All results1 test ran/service-workers/service-worker/request-end-to-end.https.html
|
Firefox (nightly channel)Testing web-platform-tests at revision c076f25 All results1 test ran/service-workers/service-worker/request-end-to-end.https.html
|
|
Thanks! lgtm |
|
Thanks for the review @mkruisselbrink! Would you mind merging this for me? |
| // enough to handle the subsequent "fetch" event. To promote test | ||
| // simplicity, the worker prevents its own termination indefinitely via a | ||
| // then-able that is never resolved. | ||
| e.waitUntil(createPending()); |
There was a problem hiding this comment.
So, this changed caused a number of other tests to fail in firefox. Consider:
- This test uses a scope of 'resources/blank.html' which many other tests also use.
- It keeps the service worker thread alive until the browser force kills it.
- We have agreement to delay marking a SW redundant until it goes idle in More clarity around waiting for an active worker to finish w3c/ServiceWorker#916
This means this change keeps the SW registration from this test alive until the force kill timeout is triggered (5 minutes on FF) and blocks any other registration from being made on the same scope.
To work around this for now I'm moving this test to a more unique scope. In the long run, though, I think this test should change to use a promise that resolves when the test explicitly sends a "done" message or something. There is no need to keep the SW alive indefinitely.
The parent document in this test first sends a message to the
ServiceWorker and later triggers a
fetchevent. Prior to this commit,the test initiated these two operations independently but required that
the Service Worker's
onmessagehandler was invoked prior to itsonfetchhandler. However, because user agents may define independenttask queues for
fetchandpostMessage[1], there the sequence inwhich the tasks are handled is not guaranteed. By incorrectly assuming
the determinacy of event order, the test created a race condition in
user agents that maintain two queues.
Re-factor the test to force a consistent execution order.
[1] From https://html.spec.whatwg.org/multipage/webappapis.html#task-queue