Summary: | Add support for postMessage buffering between the service worker and window | ||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Philip Walton <philip> | ||||||||||||||||||||||||||||||||||||
Component: | Service Workers | Assignee: | Chris Dumez <cdumez> | ||||||||||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||||||
Severity: | Normal | CC: | beidson, cdumez, commit-queue, dbates, esprehn+autocc, ews-watchlist, kangil.han, nicolas, ryanhaddad, webkit-bug-importer, youennf | ||||||||||||||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||||||||||||
Version: | Safari 12 | ||||||||||||||||||||||||||||||||||||||
Hardware: | Mac | ||||||||||||||||||||||||||||||||||||||
OS: | macOS 10.14 | ||||||||||||||||||||||||||||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=201429 | ||||||||||||||||||||||||||||||||||||||
Bug Depends on: | 201584 | ||||||||||||||||||||||||||||||||||||||
Bug Blocks: | |||||||||||||||||||||||||||||||||||||||
Attachments: |
|
Description
Philip Walton
2019-08-26 19:58:04 PDT
Created attachment 377361 [details]
WIP Patch
Completely untested.
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.
It is weird, the test in question is passing in Safari but fails in WebKitTestRunner. (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. (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. Created attachment 377398 [details]
WIP Patch
Created attachment 377402 [details]
Patch
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 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
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. (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. Created attachment 377442 [details]
Patch
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. Created attachment 377455 [details]
Patch
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 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
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 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
Created attachment 377480 [details]
Patch
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 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
Created attachment 377498 [details]
Patch
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 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
Created attachment 377721 [details]
Patch
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 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
Comment on attachment 377721 [details] Patch Clearing flags on attachment: 377721 Committed r249338: <https://trac.webkit.org/changeset/249338> All reviewed patches have been landed. Closing bug. 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 Reverted r249338 for reason: Caused 500+ layout test failures on WK1 Committed r249344: <https://trac.webkit.org/changeset/249344> (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. 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). Created attachment 377748 [details]
Patch
Created attachment 377759 [details]
Patch
Comment on attachment 377759 [details] Patch Clearing flags on attachment: 377759 Committed r249353: <https://trac.webkit.org/changeset/249353> All reviewed patches have been landed. Closing bug. Reverted r249353 for reason: The test for this change is a flaky failure. Committed r249615: <https://trac.webkit.org/changeset/249615> (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 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. (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. (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. Comment on attachment 377759 [details] Patch Clearing flags on attachment: 377759 Committed r249629: <https://trac.webkit.org/changeset/249629> All reviewed patches have been landed. Closing bug. |