1130678 - dom/broadcastchannel/tests/test_broadcastchannel_sharedWorker.html intermittent with vsync refresh driver

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Mason Chang [Inactive] [:mchang]]

From https://treeherder.mozilla.org/#/jobs?repo=try&revision=65c501042f81

b2g ics emulator opt, mochitest 2 is almost perma-orange with the vsync refresh driver.

Find and fix the problem.

Comment 1

[Mason Chang [Inactive] [:mchang]]

503 INFO TEST-UNEXPECTED-FAIL | dom/broadcastchannel/tests/test_broadcastchannel_sharedWorker.html | SharedWorker has received the message - Result logged after SimpleTest.finish()

Comment 2

[Mason Chang [Inactive] [:mchang]]

14:51:28     INFO -  502 INFO TEST-OK | dom/broadcastchannel/tests/test_broadcastchannel_sharedWorker.html | took 1693ms
14:51:29     INFO -  503 INFO TEST-UNEXPECTED-FAIL | dom/broadcastchannel/tests/test_broadcastchannel_sharedWorker.html | SharedWorker has received the message - Result logged after SimpleTest.finish()
14:51:29     INFO -  504 INFO TEST-START | dom/broadcastchannel/tests/test_broadcastchannel_worker.html
14:51:31     INFO -  505 INFO TEST-PASS | dom/broadcastchannel/tests/test_broadcastchannel_worker.html | Worker is ready!
14:51:31     INFO -  506 INFO TEST-PASS | dom/broadcastchannel/tests/test_broadcastchannel_worker.html | The message matches!
14:51:32     INFO -  507 INFO TEST-PASS | dom/broadcastchannel/tests/test_broadcastchannel_worker.html | Worker is ready!
14:51:32     INFO -  508 INFO TEST-PASS | dom/broadcastchannel/tests/test_broadcastchannel_worker.html | The message matches!

Comment 3

[Mason Chang [Inactive] [:mchang]]

Hi Andrea, I noticed you were the author of this test. Any thoughts on what we should be looking for in this test? Almost seems like there is something racy here with the workers. Thanks!

Comment 4

[Mason Chang [Inactive] [:mchang]]

I can't reproduce locally at all, I added a bunch of instrumentation and pushed to try:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5e8b28c09a70

Comment 5

[Andrea Marchesini [:baku]]

Attachment: bc.patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=93dc809b75e9

I had an idea why this happens. let me push it to try...

Comment 6

[Mason Chang [Inactive] [:mchang]]

From https://treeherder.mozilla.org/#/jobs?repo=try&revision=5e8b28c09a70

In the good case, it looks like the worker.port.onmessage executes before bc.onmessage executes. 

e.g.
https://hg.mozilla.org/try/rev/c292d4af964a#l3.23
then
https://hg.mozilla.org/try/rev/c292d4af964a#l3.33

With the vsync refresh driver, we get the inverse. The bc.onmessage executes before the worker.port.onmessage which finishes the test and fails.

Log here - http://ftp.mozilla.org/pub/mozilla.org/b2g/try-builds/[e-mail]-5e8b28c09a70/try-emulator/try_ubuntu64_vm-b2g-emulator_test-mochitest-2-bm116-tests1-linux64-build1132.txt.gz

Comment 7

[Mason Chang [Inactive] [:mchang]]

Hi Andreas,

The shared worker posts a message on both the broadcast channel and the worker [1]. Since this is in JS, this occurs on the main thread on the content process. From there, do you know if both the broadcast channel and the worker have a guaranteed order to process the message or are they on separate threads?

Thanks!

[1] https://hg.mozilla.org/try/rev/c292d4af964a#l2.9

Comment 8

[Andrea Marchesini [:baku]]

> The shared worker posts a message on both the broadcast channel and the
> worker [1]. Since this is in JS, this occurs on the main thread on the
> content process. From there, do you know if both the broadcast channel and
> the worker have a guaranteed order to process the message or are they on
> separate threads?

No garanteed order. This is the reason why I removed the second message (the worker.postMessage one).
Does my patch fix the issue? It seems so.

Comment 9

[Mason Chang [Inactive] [:mchang]]

Ahh thanks for the patch! The try from comment 6 didn't enable the vsync refresh driver, so I wasn't able to reproduce it on master. 

Here is a try push with your patch + vsync refresh driver
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7b510c38107a

Thanks for the quick response!

Comment 10

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8560841 [details] [diff] [review]
bc.patch

Looks good!

Comment 11

[Andrea Marchesini [:baku]]

It seems green enough! I think we can land this simple fix.

Comment 12

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8560841 [details] [diff] [review]
bc.patch

Review of attachment 8560841 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/broadcastchannel/tests/broadcastchannel_sharedWorker.js
@@ +1,5 @@
>  onconnect = function(evt) {
>    evt.ports[0].onmessage = function(evt) {
>      var bc = new BroadcastChannel('foobar');
>      bc.addEventListener('message', function(event) {
> +      bc.postMessage(event.data == "hello world from the window" ? 

nit: delete space.

Comment 13

[Andrea Marchesini [:baku]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/157b8136beed

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/157b8136beed