Most of the service worker events go through common code in ServiceWorkerPrivate that ensures waitUntil() and respondWith() keep the worker alive via a KeepAliveToken. Message events are different in that they use a message-specific path in WorkerPrivate. This code does not actually wait for the waitUntil() promise to resolve before discarding its KeepAliveToken. So currently ExtendableMessageEvent.waitUntil() currently does nothing. This is needed for the test in bug 1170543 to work properly.

Instead of just passing through the KeepAliveToken to the WorkerPrivate's PostMessageEventInternal() method, pass a PromiseNativeHandler that releases the token when the promise resolves. Then attach that handler to the ExtendableMessageEvent's waitUntil promise. https://treeherder.mozilla.org/#/jobs?repo=try&revision=9a52813c4d8c I'm not going to write a separate test for this since the test in bug 1170543 exercises it.

Comment on attachment 8775009 [details] [diff] [review] Make ExtendableMessageEvent.waitUntil() hold the service worker alive. r=asuth Review of attachment 8775009 [details] [diff] [review]: ----------------------------------------------------------------- (Also, I've traced that the calls where we're passing a bare `PromiseNativeHandler *` through and they all get synchronously terminated in calls to MessageEventRunnable::SetServiceWorkeData which sticks them in its RefPtr mHandler, so life-cycle is ensured by the RefPtr held on the stack by a calling ancestor.) ::: dom/workers/ServiceWorkerPrivate.cpp @@ +107,5 @@ > +namespace { > + > +class MessageWaitUntilHandler final : public PromiseNativeHandler > +{ > + nsMainThreadPtrHandle<nsISupports> mKeepAliveToken; aside: We missed out on a lot of possible mirth when mStayinAliveToken was passed over in favor of this dry naming convention. @@ +137,5 @@ > +}; > + > +NS_IMPL_ISUPPORTS0(MessageWaitUntilHandler) > + > +} nit: I believe (good) convention would be to have a "// namespace" here.

Pushed by bkelly@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/4d7badaa705c Make ExtendableMessageEvent.waitUntil() hold the service worker alive. r=asuth

Comment on attachment 8775009 [details] [diff] [review] Make ExtendableMessageEvent.waitUntil() hold the service worker alive. r=asuth Review of attachment 8775009 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/workers/ServiceWorkerPrivate.cpp @@ +107,5 @@ > +namespace { > + > +class MessageWaitUntilHandler final : public PromiseNativeHandler > +{ > + nsMainThreadPtrHandle<nsISupports> mKeepAliveToken; nit: missing one space, I think

(In reply to Andrew Sutherland [:asuth] from comment #4) > nit: missing one space, I think To late! I'll fix it in a drive by in bug 1170543.

Comment on attachment 8775009 [details] [diff] [review] Make ExtendableMessageEvent.waitUntil() hold the service worker alive. r=asuth Approval Request Comment [Feature/regressing bug #]: Service workers. [User impact if declined]: This is a web compatibility issue with chrome. It also blocks a high priority web compat issue in bug 1170543. We need to uplift to 49 in order to match when chrome will ship their version of bug 1170543. There are high profile sites that will depend on this feature. [Describe test coverage new/current, TreeHerder]: New wpt test included in bug 1170543. [Risks and why]: Minimal. Only affects service workers. [String/UUID change made/needed]: None

Comment on attachment 8775009 [details] [diff] [review] Make ExtendableMessageEvent.waitUntil() hold the service worker alive. r=asuth Improve the compatibility, taking it.