1224893 - Queue unexpected messages for subsequent calls to awaitMessage

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect)

Description

[Kris Maglione [:kmag]]

This should fix most of our intermittent failures.

Comment 1

[Kris Maglione [:kmag]]

Attachment: [webext] Queue unexpected messages for subsequent calls to awaitMessage

I can readily reproduce the test_ext_notifications.html failure on
e10s debug builds without this patch. With it, I haven't been able
to reproduce it after 400 iterations. I haven't been able to
reproduce the test_ext_simple.html or test_ext_webRequest.html
timeouts, but as far as I can tell, they only ever turned up once.

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8687638 [details] [diff] [review]
[webext] Queue unexpected messages for subsequent calls to awaitMessage

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

I have a few suggestions they're agreeable to you. This is definitely an improvement either way though.

::: testing/mochitest/tests/SimpleTest/ExtensionTestUtils.js
@@ +23,5 @@
> +
> +    let [msg, ...args] = messageQueue.shift();
> +    let listener = listenerQueue.shift();
> +
> +    // awaitMessage calls must happen in the order the messages are expected.

If that's the case, it seems like we might as well just require there be only one outstanding awaitMessage call. That way we don't need a queue of them.

However, I'm not sure we want to do that. What if we just have a queue of messages that have arrived? Both onMessage and awaitMessage would pull things out of it. That way we would avoid races with onMessage as well. I'm not too concerned about people calling awaitMessage twice and then getting messages back that arrived in a different order. It would be nice if you could do:
  yield Promise.all([ext.awaitMessage("m1"), ext.awaitMessage("m2")]);

Comment 3

[Kris Maglione [:kmag]]

(In reply to Bill McCloskey (:billm) from comment #2)
> If that's the case, it seems like we might as well just require there be
> only one outstanding awaitMessage call. That way we don't need a queue of
> them.
> 
> However, I'm not sure we want to do that. What if we just have a queue of
> messages that have arrived? Both onMessage and awaitMessage would pull
> things out of it. That way we would avoid races with onMessage as well. I'm
> not too concerned about people calling awaitMessage twice and then getting
> messages back that arrived in a different order. It would be nice if you
> could do:
>   yield Promise.all([ext.awaitMessage("m1"), ext.awaitMessage("m2")]);

Yeah, I've been coming to the conclusion that it was a bad decision. We should still be able to use |awaitMessage| even if we don't know the order messages are going to come in.

Comment 4

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/fx-team/rev/eb184c427415f118f8d63ea3254a17c6ba8a7b29
Bug 1224893: [webext] Queue unexpected messages for subsequent calls to awaitMessage

Comment 5

[Kris Maglione [:kmag]]

I went with ignoring the order of awaitEvent calls, but still resolving await listeners strictly in the order messages come in. I meant to push that to review... I'm not sure why fx-team accepted it without an r= tag.

Comment 6

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/fx-team/rev/55585709276f4b9c00375a1ec8075198e6d8960f
Bug 1224893: [webext] Queue unexpected messages for subsequent calls to awaitMessage, follow-up. r=me r=billm

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/eb184c427415
https://hg.mozilla.org/mozilla-central/rev/55585709276f

Comment 8

[Liz Henry (:lizzard) (relman/hg->git project)]

Let's uplift this fix to aurora and beta to take care of the test failures from bug 1226423.

Comment 9

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8687638 [details] [diff] [review]
[webext] Queue unexpected messages for subsequent calls to awaitMessage

Please uplift to aurora and beta to fix an intermittent test failure.

Comment 10

[Kris Maglione [:kmag]]

I already pushed an uplift commit to try for aurora: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8bdea36a72fa

Uplifting to beta is probably a good idea, but I don't think the tests for bug 1226423 will apply on beta either way. It would be better not to uplift them.

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/837e564f3697
https://hg.mozilla.org/releases/mozilla-aurora/rev/7d2f0faddb3f

Comment 12

[Liz Henry (:lizzard) (relman/hg->git project)]

OK, then let's back the tests out of beta. Thanks Kris.

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/5dc47809b28c
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/3007bf388ce3

Comment 14

[Phil Ringnalda (:philor)]

Backed out of beta in https://hg.mozilla.org/releases/mozilla-beta/rev/89a4374c8f67 - since the syntax error in https://treeherder.mozilla.org/logviewer.html#?job_id=663116&repo=mozilla-beta seems nonsensical I assume it actually means you're doing something unsupported on 43 or on release_build.

Comment 15

[Kris Maglione [:kmag]]

I guess we don't have support for destructuring with spread expressions in 43. I'll back port if this becomes necessary on beta again.

Comment 16

[Liz Henry (:lizzard) (relman/hg->git project)]

Marking wontfix for 43 at this point.