Summary: | Safari 11.1: MessageChannel no longer works between Workers | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ashley Gullen <ashley> | ||||||
Component: | WebKit Misc. | Assignee: | Tadeu Zagallo <tzagallo> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Critical | CC: | ap, beidson, cdumez, commit-queue, dbates, esprehn+autocc, ews-watchlist, ggaren, hy, kangil.han, ryanhaddad, tzagallo, webkit-bug-importer, youennf | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | Safari 11 | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
See Also: |
https://bugs.webkit.org/show_bug.cgi?id=184254 https://bugs.webkit.org/show_bug.cgi?id=184769 |
||||||||
Attachments: |
|
Description
Ashley Gullen
2018-04-11 08:27:12 PDT
I'd add it appeared to work in Safari 11.0 so this appears to be a regression. Potential dupe of Bug 184254? (In reply to Chris Dumez from comment #3) > Potential dupe of Bug 184254? I reduced the test case and I don't believe it's a dupe. This issue only appears when sending messages from workers to workers, and is not affected by the kind of data being sent. I will investigate more. Created attachment 337908 [details]
Patch
The issue was that when transferring a MessagePort, we only retained the MessagePortChannel for the duration of the transfer if it was being sent via another MessagePort. When sending both ports via workers, there was no reference to the MessagePortChannel left, so it got deallocated. By the time the message was received, the reference from the port back to the channel had been cleared, so the message was dropped. I added a minimal test case, and also checked the example provided, both work. I'm not sure if entangling/disentangling is the best place to retain the channel though, but it seemed like the most central place, where it would require the least changes. Created attachment 337938 [details]
Patch
Comment on attachment 337938 [details]
Patch
r=me
Comment on attachment 337938 [details] Patch Clearing flags on attachment: 337938 Committed r230738: <https://trac.webkit.org/changeset/230738> All reviewed patches have been landed. Closing bug. The LayoutTest added with this change is a flaky failure due to messages being received/printed out of order: --- /Volumes/Data/slave/sierra-release-tests-wk1/build/layout-test-results/workers/worker-to-worker-expected.txt +++ /Volumes/Data/slave/sierra-release-tests-wk1/build/layout-test-results/workers/worker-to-worker-actual.txt @@ -6,6 +6,6 @@ PASS successfullyParsed is true TEST COMPLETE +[Worker 2] Received message: hello [Worker 1] Received message: hello -[Worker 2] Received message: hello https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=workers%2Fworker-to-worker.html Comment on attachment 337938 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=337938&action=review > LayoutTests/workers/worker-to-worker.html:23 > + worker("Worker 1", mc.port1, 1), Looks like the problem here is that 1ms is not a deterministic or reliable guarantee of ordering. If you change the worker function to pass e.data to its resolver, then you can process log the e.data results in deterministic order at the very end, after all promises have resolved. Tadeu has a fix in bug 184769. The Status says "Resolved Fixed" but i'm still facing the same issue with current versions (macOS 10.13.6/Safari 11.1.2 & iOS 11.4). Posting messages from worker to worker using messagechannel ports won't work at all. You can still use Ashley's Repro URL to reproduce this in the mentioned versions. Apple are typically opaque about process and slow to get fixes to release. However as far as I can tell by myself, it should be fixed in the latest Safari Technology Preview, and in Safari 12 when that's released. Yes, this feature has been broken for the entire time Safari 11.1 has been out. They did the same thing when they broke WebAssembly... Thanks for the info Ashley! I tested it with Safari Tech Preview now and can confirm that it's working.. Unfortunately i will need to build a workaround if this fix only ships with 12 since we found out that most of our macOS/IOs users don't update their devices/systems frequently. |