Bug 201169 – Add support for postMessage buffering between the service worker and window

• Component: Service Workers
• Status: RESOLVED FIXED

[Philip Walton]

Safari 12 implements the `ServiceWorkerContainer.startMessages()` API [1], but it does not actually implement the underlying buffering of messages sent from the service worker to the window in the `fetch` event for navigation requests [2].

Firefox has supported buffering for a while, and Chrome added support in M74 [3]. Prior to Chrome supporting this feature, we (on the Chrome team) recommended feature detecting support via the presence of the `startMessages()` method (since its purpose is to allow the buffer to start clearing before the DOMContentLoaded event):

```
if (navigator.serviceWorker && navigator.serviceWorker.startMessages) {
  // Assuming the service worker supports message buffering
}
```

But given that Safari has the `startMessages()` method but does not implement the underlying buffering behavior, the above will result in a false positive, and it will likely mean lots of missed message from the service worker after navigation requests.

The ability to reliably send a message from the service worker to the window after new navigations is important, as developers commonly rely on postMessage to inform the window that a newer version of the page exists [4]. If these messages are being dropped in Safari, then Safari users may be seeing outdated content and not receiving the message that new content is available.


[1] https://w3c.github.io/ServiceWorker/#dom-serviceworkercontainer-startmessages

[2] https://twitter.com/chris_dumez/status/1166170761125490689

[3] https://www.chromestatus.com/features/5751307839209472

[4] https://developers.google.com/web/tools/workbox/modules/workbox-broadcast-cache-update

[Chris Dumez]

Created attachment 377361 [details]
WIP Patch

Completely untested.

[Chris Dumez]

Created attachment 377389 [details]
WIP Patch

Still trying to get imported/w3c/web-platform-tests/service-workers/service-worker/postmessage-to-client-message-queue.https.html  to pass.

[Chris Dumez]

It is weird, the test in question is passing in Safari but fails in WebKitTestRunner.

[Chris Dumez]

(In reply to Chris Dumez from comment #3)
> It is weird, the test in question is passing in Safari but fails in
> WebKitTestRunner.

Also passes in MiniBrowser. I suspect a testharness issue here.

[Chris Dumez]

(In reply to Chris Dumez from comment #4)
> (In reply to Chris Dumez from comment #3)
> > It is weird, the test in question is passing in Safari but fails in
> > WebKitTestRunner.
> 
> Also passes in MiniBrowser. I suspect a testharness issue here.

TestRunner::notifyDone() gets called but it does not seem to end the test. I think the test runner may be waiting for the page to finish loading but it never does.

[Chris Dumez]

Created attachment 377398 [details]
WIP Patch

[Chris Dumez]

Created attachment 377402 [details]
Patch

[EWS Watchlist]

Comment on attachment 377402 [details]
Patch

Attachment 377402 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12975185

New failing tests:
http/wpt/html/semantics/text-level-semantics/the-a-element/a-download-click-404.html

[EWS Watchlist]

Created attachment 377414 [details]
Archive of layout-test-results from ews212 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews212  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[youenn fablet]

Comment on attachment 377402 [details]
Patch

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

> Source/WebCore/ChangeLog:16
> +        While the message queue is disabled, messages from posted by the service worker

s/posted by//

> Source/WebCore/ChangeLog:17
> +        to the client simply get queued and we only get processed once the queue gets

s/we//

> Source/WebCore/workers/service/ServiceWorkerContainer.cpp:485
> +    m_messageQueue.enqueueEvent(WTFMove(messageEvent));

This enqueues an event instead of dispatching it, probably delaying the event dispatch to the next run loop.
I wonder whether this might create some issues with event ordering like we had in the past.
It seems that we could remove the postTask call in SWClientConnection::postMessageToServiceWorkerClient to fix that.

[Chris Dumez]

(In reply to youenn fablet from comment #10)
> Comment on attachment 377402 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=377402&action=review
> 
> > Source/WebCore/ChangeLog:16
> > +        While the message queue is disabled, messages from posted by the service worker
> 
> s/posted by//
> 
> > Source/WebCore/ChangeLog:17
> > +        to the client simply get queued and we only get processed once the queue gets
> 
> s/we//
> 
> > Source/WebCore/workers/service/ServiceWorkerContainer.cpp:485
> > +    m_messageQueue.enqueueEvent(WTFMove(messageEvent));
> 
> This enqueues an event instead of dispatching it, probably delaying the
> event dispatch to the next run loop.
> I wonder whether this might create some issues with event ordering like we
> had in the past.
> It seems that we could remove the postTask call in
> SWClientConnection::postMessageToServiceWorkerClient to fix that.

The postTask in SWClientConnection::postMessageToServiceWorkerClient() will be useful once we support workers being clients though, but OK for now.

[Chris Dumez]

Created attachment 377442 [details]
Patch

[youenn fablet]

Comment on attachment 377442 [details]
Patch

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

> Source/WebCore/dom/Document.cpp:324
> +#if ENABLE(SERVICE_WORKER)

No need for ENABLE(SERVICE_WORKER), ServiceWorkerContainer.h is doing it itself.

> Source/WebCore/workers/service/SWClientConnection.cpp:129
> +        container->postMessage(WTFMove(message), WTFMove(sourceData), WTFMove(sourceOrigin));

If we want to keep the fact that we are actually posting a task to dispatch a message event, we could rename this postMessage method.

[Chris Dumez]

Created attachment 377455 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 377455 [details]
Patch

Rejecting attachment 377455 [details] from commit-queue.

Number of test failures exceeded the failure limit.
Full output: https://webkit-queues.webkit.org/results/12977015

[WebKit Commit Bot]

Created attachment 377462 [details]
Archive of layout-test-results from webkit-cq-03 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the commit-queue.
Bot: webkit-cq-03  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 377455 [details]
Patch

Attachment 377455 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12977176

New failing tests:
js/error-should-not-strong-reference-global-object.html
http/wpt/html/semantics/text-level-semantics/the-a-element/a-download-click-404.html

[EWS Watchlist]

Created attachment 377468 [details]
Archive of layout-test-results from ews210 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews210  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[Chris Dumez]

Created attachment 377480 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 377480 [details]
Patch

Rejecting attachment 377480 [details] from commit-queue.

Number of test failures exceeded the failure limit.
Full output: https://webkit-queues.webkit.org/results/12977857

[WebKit Commit Bot]

Created attachment 377491 [details]
Archive of layout-test-results from webkit-cq-01 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the commit-queue.
Bot: webkit-cq-01  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[Chris Dumez]

Created attachment 377498 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 377498 [details]
Patch

Rejecting attachment 377498 [details] from commit-queue.

Number of test failures exceeded the failure limit.
Full output: https://webkit-queues.webkit.org/results/12978406

[WebKit Commit Bot]

Created attachment 377514 [details]
Archive of layout-test-results from webkit-cq-03 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the commit-queue.
Bot: webkit-cq-03  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[Chris Dumez]

Created attachment 377721 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 377721 [details]
Patch

Rejecting attachment 377721 [details] from commit-queue.

Number of test failures exceeded the failure limit.
Full output: https://webkit-queues.webkit.org/results/12984563

[WebKit Commit Bot]

Created attachment 377727 [details]
Archive of layout-test-results from webkit-cq-03 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the commit-queue.
Bot: webkit-cq-03  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[Chris Dumez]

Comment on attachment 377721 [details]
Patch

Clearing flags on attachment: 377721

Committed r249338: <https://trac.webkit.org/changeset/249338>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/54891287>

[Ryan Haddad]

https://trac.webkit.org/changeset/249338 seems to have caused hundreds of test failures on macOS WK1: https://build.webkit.org/builders/Apple%20High%20Sierra%20Release%20WK1%20(Tests)/builds/15233

[Ryan Haddad]

Reverted r249338 for reason:

Caused 500+ layout test failures on WK1

Committed r249344: <https://trac.webkit.org/changeset/249344>

[Chris Dumez]

(In reply to Ryan Haddad from comment #32)
> Reverted r249338 for reason:
> 
> Caused 500+ layout test failures on WK1
> 
> Committed r249344: <https://trac.webkit.org/changeset/249344>

Thanks, will investigate.

[Chris Dumez]

Comment on attachment 377721 [details]
Patch

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

> LayoutTests/resources/testharnessreport.js:101
> +            testRunner.forceImmediateCompletion();

It must be this change that is somehow confusing DumpRenderTree (tests output gets compared with the test expectations of the previous or next test).

[Chris Dumez]

Created attachment 377748 [details]
Patch

[Chris Dumez]

Created attachment 377759 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 377759 [details]
Patch

Clearing flags on attachment: 377759

Committed r249353: <https://trac.webkit.org/changeset/249353>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

Reverted r249353 for reason:

The test for this change is a flaky failure.

Committed r249615: <https://trac.webkit.org/changeset/249615>

[Ryan Haddad]

(In reply to Ryan Haddad from comment #39)
> Reverted r249353 for reason:
> 
> The test for this change is a flaky failure.
> 
> Committed r249615: <https://trac.webkit.org/changeset/249615>
See https://bugs.webkit.org/show_bug.cgi?id=201429#c0

[Chris Dumez]

The reason the layout test is flaky is the same as for a lot of our other layout tests. When the test unregisters then re-registers a service worker, they sometimes get the same registration because unregistering merely does a "tryClear" not a "clear" of the registration.

If I do this, then the flakiness goes away:
diff --git a/Source/WebCore/workers/service/server/SWServerJobQueue.cpp b/Source/WebCore/workers/service/server/SWServerJobQueue.cpp
index 4ac244aee65..5fe62356e8f 100644
--- a/Source/WebCore/workers/service/server/SWServerJobQueue.cpp
+++ b/Source/WebCore/workers/service/server/SWServerJobQueue.cpp
@@ -320,7 +320,7 @@ void SWServerJobQueue::runUnregisterJob(const ServiceWorkerJobData& job)
     m_server.resolveUnregistrationJob(job, m_registrationKey, true);
 
     // Invoke Try Clear Registration with registration.
-    registration->tryClear();
+    registration->clear();
     finishCurrentJob();
 }


But then we no longer match the spec. I will look into it more.

[Chris Dumez]

(In reply to Chris Dumez from comment #41)
> The reason the layout test is flaky is the same as for a lot of our other
> layout tests. When the test unregisters then re-registers a service worker,
> they sometimes get the same registration because unregistering merely does a
> "tryClear" not a "clear" of the registration.
> 
> If I do this, then the flakiness goes away:
> diff --git a/Source/WebCore/workers/service/server/SWServerJobQueue.cpp
> b/Source/WebCore/workers/service/server/SWServerJobQueue.cpp
> index 4ac244aee65..5fe62356e8f 100644
> --- a/Source/WebCore/workers/service/server/SWServerJobQueue.cpp
> +++ b/Source/WebCore/workers/service/server/SWServerJobQueue.cpp
> @@ -320,7 +320,7 @@ void SWServerJobQueue::runUnregisterJob(const
> ServiceWorkerJobData& job)
>      m_server.resolveUnregistrationJob(job, m_registrationKey, true);
>  
>      // Invoke Try Clear Registration with registration.
> -    registration->tryClear();
> +    registration->clear();
>      finishCurrentJob();
>  }
> 
> 
> But then we no longer match the spec. I will look into it more.

I think I found where we do not match the spec and why we are flaky here. I am working on fixing it.

[Chris Dumez]

(In reply to Chris Dumez from comment #42)
> (In reply to Chris Dumez from comment #41)
> > The reason the layout test is flaky is the same as for a lot of our other
> > layout tests. When the test unregisters then re-registers a service worker,
> > they sometimes get the same registration because unregistering merely does a
> > "tryClear" not a "clear" of the registration.
> > 
> > If I do this, then the flakiness goes away:
> > diff --git a/Source/WebCore/workers/service/server/SWServerJobQueue.cpp
> > b/Source/WebCore/workers/service/server/SWServerJobQueue.cpp
> > index 4ac244aee65..5fe62356e8f 100644
> > --- a/Source/WebCore/workers/service/server/SWServerJobQueue.cpp
> > +++ b/Source/WebCore/workers/service/server/SWServerJobQueue.cpp
> > @@ -320,7 +320,7 @@ void SWServerJobQueue::runUnregisterJob(const
> > ServiceWorkerJobData& job)
> >      m_server.resolveUnregistrationJob(job, m_registrationKey, true);
> >  
> >      // Invoke Try Clear Registration with registration.
> > -    registration->tryClear();
> > +    registration->clear();
> >      finishCurrentJob();
> >  }
> > 
> > 
> > But then we no longer match the spec. I will look into it more.
> 
> I think I found where we do not match the spec and why we are flaky here. I
> am working on fixing it.

Addressing flakiness via Bug 201584.

[Chris Dumez]

Comment on attachment 377759 [details]
Patch

Clearing flags on attachment: 377759

Committed r249629: <https://trac.webkit.org/changeset/249629>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.