Closed Bug 1224893 Opened 4 years ago Closed 4 years ago

Queue unexpected messages for subsequent calls to awaitMessage

Categories

(WebExtensions :: Untriaged, defect)

defect
Not set

Tracking

(firefox43 wontfix, firefox44 fixed, firefox45 fixed)

RESOLVED FIXED
mozilla45
Iteration:
45.2 - Nov 30
Tracking Status
firefox43 --- wontfix
firefox44 --- fixed
firefox45 --- fixed

People

(Reporter: kmag, Assigned: kmag)

References

Details

Attachments

(1 file)

This should fix most of our intermittent failures.
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.
Attachment #8687638 - Flags: review?(wmccloskey)
Iteration: --- → 44.3 - Nov 2
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")]);
Attachment #8687638 - Flags: review?(wmccloskey) → review+
(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.
https://hg.mozilla.org/integration/fx-team/rev/eb184c427415f118f8d63ea3254a17c6ba8a7b29
Bug 1224893: [webext] Queue unexpected messages for subsequent calls to awaitMessage
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.
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
https://hg.mozilla.org/mozilla-central/rev/eb184c427415
https://hg.mozilla.org/mozilla-central/rev/55585709276f
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Let's uplift this fix to aurora and beta to take care of the test failures from bug 1226423.
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.
Attachment #8687638 - Flags: approval-mozilla-beta+
Attachment #8687638 - Flags: approval-mozilla-aurora+
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.
OK, then let's back the tests out of beta. Thanks Kris.
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.
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.
Iteration: 44.3 - Nov 2 → 45.2 - Nov 30
Marking wontfix for 43 at this point.
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.