1263734 - Implement navigator.serviceWorkers.startMessages()

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect, P2)

Description

[Ben Kelly [:bkelly, not reviewing]]

See:

  https://github.com/slightlyoff/ServiceWorker/issues/728

Comment 1

[Yaron Tausky [:ytausky]]

Attachment: Bug 1263734: Add WPT test for ServiceWorkerContainer.startMessages()

There is currently no test that verifies the existence of this method, nor
one that verifies it works correctly. It is expected to fail since Gecko
doesn't implement it yet.

Comment 2

[Yaron Tausky [:ytausky]]

Attachment: Bug 1263734: Add stub for ServiceWorkerContainer.startMessages()

Some existing service worker tests that rely on Client.postMessage() will
break when that method is implemented according to the spec. The breakage
can be fixed by calling startMessages(), yet this method doesn't exist yet.
Adding an empty stub makes it possible to fix the affected tests before the
logic changes land.

Comment 3

[Yaron Tausky [:ytausky]]

Attachment: Bug 1263734: Add startMessages() calls to service worker-related WPT tests

The service workers spec defines the queuing of messages sent via Client.postMessage().
In particular, it mandates that these messages are only dispatched after a call to
ServiceWorkerContainer.startMessages() or an assignment to its onmessage handler. The
tests in this commit needed to be modified in order to work correctly under a conformat
implementation.

Note: in cases where startMessages() is called before an assignment to onmessage, the
test relied on messages being discarded before the event handler was set.

Comment 4

[Yaron Tausky [:ytausky]]

Attachment: Bug 1263734: Add startMessages() calls in a bunch of Mochi tests

The service workers spec defines the queuing of messages sent via Client.postMessage().
In particular, it mandates that these messages are only dispatched after a call to
ServiceWorkerContainer.startMessages() or an assignment to its onmessage handler. The
tests in this commit needed to be modified in order to work correctly under a conformat
implementation.

Comment 5

[Yaron Tausky [:ytausky]]

Attachment: Bug 1263734: Implement ServiceWorkerContainer.startMessages()

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Yaron Tausky [:ytausky] from comment #3)
> Created attachment 9003811 [details]
> Bug 1263734: Add startMessages() calls to service worker-related WPT tests
> 
> The service workers spec defines the queuing of messages sent via
> Client.postMessage().
> In particular, it mandates that these messages are only dispatched after a
> call to
> ServiceWorkerContainer.startMessages() or an assignment to its onmessage
> handler. The
> tests in this commit needed to be modified in order to work correctly under
> a conformat
> implementation.
> 
> Note: in cases where startMessages() is called before an assignment to
> onmessage, the
> test relied on messages being discarded before the event handler was set.

The fact that so many WPT tests had to be modified makes me wonder if this would be a web compatible change.  Do you think perhaps we should reconsider the API at the spec level?

Comment 7

[Yaron Tausky [:ytausky]]

Looking again at the discussion on GitHub, I noticed I completely missed the associated changes to the HTML standard (which the SW standard unfortunately doesn't mention at all). Let's see what the outcome is when messages are enabled on document loading.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Yea, I think auto-starting message delivery on document load would reduce the compat impact.  I forgot about that as well.  I expect you'll only have a few tests, like the about-blank-replacement stuff, that will need to have startMessages().  Thanks!

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2c01f22661fa
Implement ServiceWorkerContainer.startMessages() r=asuth,smaug

Comment 10

[Dorel Luca [:dluca]]

Backed out changeset 2c01f22661fa (bug 1263734) for wpt failures in fetch/api/request/destination/fetch-destination-no-load-event.https.html. CLOSED TREE

Log:

https://treeherder.mozilla.org/logviewer.html#?job_id=204522914&repo=autoland&lineNumber=2162

 INFO - TEST-OK | /fetch/api/request/destination/fetch-destination-iframe.https.html | took 636ms
[task 2018-10-10T14:59:18.073Z] 14:59:18     INFO - TEST-START | /fetch/api/request/destination/fetch-destination-no-load-event.https.html
[task 2018-10-10T14:59:28.219Z] 14:59:28     INFO - 
[task 2018-10-10T14:59:28.224Z] 14:59:28     INFO - TEST-PASS | /fetch/api/request/destination/fetch-destination-no-load-event.https.html | Initialize global state 
[task 2018-10-10T14:59:28.224Z] 14:59:28     INFO - TEST-UNEXPECTED-TIMEOUT | /fetch/api/request/destination/fetch-destination-no-load-event.https.html | Background image fetches with an "image" Request.destination - Test timed out
[task 2018-10-10T14:59:28.225Z] 14:59:28     INFO - 
[task 2018-10-10T14:59:28.225Z] 14:59:28     INFO - TEST-UNEXPECTED-NOTRUN | /fetch/api/request/destination/fetch-destination-no-load-event.https.html | Font loading API fetches with an "font" Request.destination - expected PASS
[task 2018-10-10T14:59:28.225Z] 14:59:28     INFO - 
[task 2018-10-10T14:59:28.226Z] 14:59:28     INFO - TEST-UNEXPECTED-NOTRUN | /fetch/api/request/destination/fetch-destination-no-load-event.https.html | CSS font fetches with an "font" Request.destination - expected PASS
[task 2018-10-10T14:59:28.226Z] 14:59:28     INFO - 
[task 2018-10-10T14:59:28.226Z] 14:59:28     INFO - TEST-UNEXPECTED-NOTRUN | /fetch/api/request/destination/fetch-destination-no-load-event.https.html | sendBeacon() fetches with an empty string Request.destination - expected PASS
[task 2018-10-10T14:59:28.227Z] 14:59:28     INFO - 
[task 2018-10-10T14:59:28.227Z] 14:59:28     INFO - TEST-UNEXPECTED-NOTRUN | /fetch/api/request/destination/fetch-destination-no-load-event.https.html | Cache.add() fetches with an empty string Request.destination - expected PASS
[task 2018-10-10T14:59:28.227Z] 14:59:28     INFO - 
[task 2018-10-10T14:59:28.227Z] 14:59:28     INFO - TEST-UNEXPECTED-NOTRUN | /fetch/api/request/destination/fetch-destination-no-load-event.https.html | importScripts() fetches with a "script" Request.destination - expected PASS
[task 2018-10-10T14:59:28.228Z] 14:59:28     INFO - 
[task 2018-10-10T14:59:28.228Z] 14:59:28     INFO - TEST-UNEXPECTED-NOTRUN | /fetch/api/request/destination/fetch-destination-no-load-event.https.html | @import fetches with a "style" Request.destination - expected PASS
[task 2018-10-10T14:59:28.228Z] 14:59:28     INFO - TEST-UNEXPECTED-TIMEOUT | /fetch/api/request/destination/fetch-destination-no-load-event.https.html | expected OK
[task 2018-10-10T14:59:28.229Z] 14:59:28     INFO - TEST-INFO took 10154ms
[task 2018-10-10T14:59:28.253Z] 14:59:28     INFO - PID 6362 | 1539183568245	Marionette	INFO	Stopped listening on port 2828
[task 2018-10-10T14:59:28.354Z] 14:59:28     INFO - PID 6362 | [Parent 6362, Gecko_IOThread] WARNING: pipe error (89): Connection reset by peer: file /builds/worker/workspace/build/src/ipc/chromium/src/chrome/common/ipc_channel_posix.cc, line 363
[task 2018-10-10T14:59:28.741Z] 14:59:28     INFO - Browser exited with return code 0
[task 2018-10-10T14:59:28.742Z] 14:59:28  WARNING - u'runner_teardown': ()
[task 2018-10-10T14:59:28.744Z] 14:59:28     INFO - Closing logging queue
[task 2018-10-10T14:59:28.745Z] 14:59:28     INFO - queue closed
[task 2018-10-10T14:59:28.769Z] 14:59:28     INFO - Setting up ssl
[task 2018-10-10T14:59:28.809Z] 14:59:28     INFO - certutil | 
[task 2018-10-10T14:59:28.857Z] 14:59:28     INFO - certutil | 
[task 2018-10-10T14:59:28.873Z] 14:59:28     INFO - certutil | 
[task 2018-10-10T14:59:28.874Z] 14:59:28     INFO - Certificate Nickname                                         Trust Attributes
[task 2018-10-10T14:59:28.874Z] 14:59:28     INFO -                                                              SSL,S/MIME,JAR/XPI
[task 2018-10-10T14:59:28.874Z] 14:59:28     INFO - 
[task 2018-10-10T14:59:28.875Z] 14:59:28     INFO - web-platform-tests                                           CT,, 
[task 2018-10-10T14:59:28.875Z] 14:59:28     INFO - 
[task 2018-10-10T14:59:28.891Z] 14:59:28     INFO - Application command: /builds/worker/workspace/build/application/firefox/firefox --marionette about:blank -profile /tmp/tmppdmAEW.mozrunner
[task 2018-10-10T14:59:28.907Z] 14:59:28     INFO - Starting runner
[task 2018-10-10T14:59:30.981Z] 14:59:30     INFO - PID 6590 | 1539183570978	Marionette	INFO	Listening on port 2828
[task 2018-10-10T14:59:31.306Z] 14:59:31     INFO - TEST-START | /fetch/api/request/destination/fetch-destination-worker.https.html
[task 2018-10-10T14:59:31.939Z] 14:59:31     INFO - PID 6590 | ###!!! [Parent][DispatchAsyncMessage] Error: PClientSourceOp::Msg___delete__ Route error: message sent to unknown actor ID
[task 2018-10-10T14:59:31.947Z] 14:59:31     INFO - PID 6590 | ###!!! [Parent][DispatchAsyncMessage] Error: PClientSourceOp::Msg___delete__ Route error: message sent to unknown actor ID
[task 2018-10-10T14:59:31.949Z] 14:59:31     INFO - ..
[task 2018-10-10T14:59:31.949Z] 14:59:31     INFO - TEST-OK | /fetch/api/request/destination/fetch-destination-worker.https.html | took 646ms
[task 2018-10-10T14:59:31.951Z] 14:59:31     INFO - TEST-START | /fetch/api/request/destination/fetch-destination.https.html
[task 2018-10-10T14:59:32.431Z] 14:59:32     INFO - ..........................
[task 2018-10-10T14:59:32.432Z] 14:59:32     INFO - TEST-OK | /fetch/api/request/destination/fetch-destination.https.html | took 481ms

Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&selectedJob=204522914&revision=2c01f22661fabd177c81e658e93319bc4faba8df

Backout:
https://hg.mozilla.org/integration/autoland/rev/db4990bc9b4cefd8f4e85a17de49796093cf6e90

Comment 11

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/13456 for changes under testing/web-platform/tests

Comment 12

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 13

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR was closed without merging

Comment 14

[Yaron Tausky [:ytausky]]

I found the problem and I'm running a revised patch on try to make sure it's solved.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cecf523a14b1
Implement ServiceWorkerContainer.startMessages() r=asuth,smaug

Comment 16

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/cecf523a14b1

Comment 17

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Can't merge web-platform-tests PR due to failing upstream checks:
Github PR https://github.com/web-platform-tests/wpt/pull/13456
* Taskcluster (pull_request) (https://tools.taskcluster.net/task-group-inspector/#/GEK9YCLkR6ach08CHChzlA)

Comment 18

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged

Comment 19

[Chris Mills [:cmills]]

Note to docs team:

I've added a note about this to the Fx64 rel notes:
https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/64#APIs

When we document this, we need to write the method page and update the compat data.

Comment 20

[Chris Mills [:cmills]]

Hi there,

I've just come to document this, and I'm confused — I don't really understand how this method works.

I mean, I've read the bug thread and associated GH issues, but when I try to put together a quick demo to test this out, messages sent to/from the SW via postmessage() seem to be sent anyway, regardless of whether I invoke this method or not.

Can you provide me with a brief code snippet or two showing how you'd use this? And explain in exactly what circumstances it is needed?

Thanks in advance.

Comment 21

[Yaron Tausky [:ytausky]]

It's kinda tricky, since it's meant to solve a race condition that happens while loading a page: if the service worker posts a message to the page while the page is loading, the message might get lost if it arrived before the part of the page that installs the event handler was run. The only time you can observe a difference in behavior after calling startMessages() is before the page finished loading, and causing this race condition reliably (i.e. in a way suitable for a demonstration) is quite intricate. You can look at this test to see what I mean: https://searchfox.org/mozilla-central/source/testing/web-platform/tests/service-workers/service-worker/postmessage-to-client-message-queue.https.html
It'll probably only confuse whoever read it. I think the most useful thing to say here is to mention the possible race condition, and specifying when messages start getting dispatched:
1. When navigator.serviceWorker.onmessage is set
2. When navigator.serviceWorker.startMessages() is called
3. When the page finishes loading
Any message that arrived before that is stored and will get dispatched when one of the events above occurs.

Comment 22

[Chris Mills [:cmills]]

Thanks Yaron, that is really helpful!

So, I've documented the method:
https://developer.mozilla.org/en-US/docs/Web/API/ServiceWorkerContainer
https://developer.mozilla.org/en-US/docs/Web/API/ServiceWorkerContainer/startMessages

And submitted a PR to update the Fx compat data:
https://github.com/mdn/browser-compat-data/pull/3129

Please let me know if my description on the ref page looks OK. Thanks!

Comment 23

[Yaron Tausky [:ytausky]]

Two things stood out to me:

First of all, there are specific mentions of `ServiceWorkerContainer.onmessage`; I don't know what MDN's conventions are, so it might be a nuance that's not worth mentioning, but there is a semantic difference between calling `navigator.serviceWorker.addEventHandler('message', handler1)` and `navigator.serviceWorker.onmessage = handler2` before the page is loaded, namely that messages will start getting handled by handler2 after setting onmessage directly (even before the page finished loading), but handler1 will only get called after the page finished loading, unless `navigator.serviceWorker.startMessages()` has been called. I guess the problem here is that by only mentioning `onmessage`, users of addEventHandler might expect the exact same behavior.

The second thing is that the explanation in startMessages's page is somewhat incorrect. The race condition is between a service worker sending a message to a controlled page while that page is loading, and not as described there. Once a page finishes loading all messages will be dispatched -- if the page didn't install a handler during loading, all messages from the service worker will be dropped until a handler is installed. This is okay, since the page had a designated time frame in which it could install a handler and was guaranteed to receive all messages if it did.

Maybe a less confusing way to explain this to a user would be something like this: "By default, all messages sent from a page's controlling service worker to the page (using Client.postMessage()) are queued while the page is loading, and get dispatched once the page finishes loading. It's possible to start dispatching these messages earlier by calling ServiceWorkerContainer.startMessages(). This is done automatically when setting ServiceWorkerContainer.onmessage directly."

The race condition used to exist, but I think talking about it now that it's no longer there is better left as a footnote for the interested. It was solved by queuing the messages until the page is loaded, and startMessages() was added to let pages react more quickly, by indicating the they are ready to handle messages even before they're fully loaded. From a user's perspective, this is a function that makes their page react to messages earlier, so they don't need to think about race conditions at all.

[I apologize if this is getting lengthy, I'm not used to thinking a lot about documentation... :-)]

Comment 24

[Chris Mills [:cmills]]

(In reply to Yaron Tausky [:ytausky] from comment #23)

> [I apologize if this is getting lengthy, I'm not used to thinking a lot
> about documentation... :-)]

Not at all — this is really helpful! OK, I think I understand what's going on now, and have updated the page accordingly, largely using your text. Care to give it another check? ;-)

Comment 25

[Yaron Tausky [:ytausky]]

Looks good to me!

Comment 26

[Matt Falkenhagen]

Thanks for the excellent WPT for this! Very thorough and clear even though it's testing something super complex.