Closed Bug 1208687 Opened 4 years ago Closed 4 years ago

Make sure we're clearing the right queue


(Core :: DOM: Workers, defect)

Not set



Tracking Status
firefox42 --- fixed
firefox43 --- fixed
firefox44 --- fixed
firefox-esr38 --- affected


(Reporter: khuey, Assigned: khuey)




(1 file)

Attached patch PatchSplinter Review
See the patch's commit messages
Attachment #8666288 - Flags: review?(ehsan)
Comment on attachment 8666288 [details] [diff] [review]

Review of attachment 8666288 [details] [diff] [review]:

Thanks for the fix!
Attachment #8666288 - Flags: review?(ehsan) → review+
Unfortunately, mochitest-8 on the b2g emulator is still orange with this patch:
Blocks: 1204596
This has caused W(4) bustages like this /workers/interfaces/WorkerGlobalScope/close/incoming-message.html | close() and incoming message - expected FAIL

Can you fix or shall I backout?
Flags: needinfo?(khuey)
That's an unexpected pass ... easy to fix.
Flags: needinfo?(khuey)
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Comment on attachment 8666288 [details] [diff] [review]

We should take this on branches because it fixes a violation of an important invariant in the worker code.  This bug could lead to random instability and crashes.

There are no string or UUID changes in this patch.  The flaw was introduced in bug 914762 in Firefox 29.
Attachment #8666288 - Flags: approval-mozilla-esr38?
Attachment #8666288 - Flags: approval-mozilla-beta?
Attachment #8666288 - Flags: approval-mozilla-aurora?
Kyle, could you fill the uplift template? In particular, we need the risk evaluation (especially for esr)
Flags: needinfo?(khuey)
Comment on attachment 8666288 [details] [diff] [review]

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: Prevents a violation of a key invariant in workers code, which may cause random instability and crashes.
User impact if declined: Random instability
Fix Landed on Version: 44
Risk to taking this patch (and alternatives if risky): Low risk. The alternative is to do nothing and accept a potentially higher crash rate.
String or UUID changes made by this patch: N/A

See for more info.
Flags: needinfo?(khuey)
Comment on attachment 8666288 [details] [diff] [review]

OK, looks like it will improve the overall quality of Firefox.
Taking it for 42 & 43. Should be in 42 beta 4.

About esr38, it doesn't match the criteria for uplift. We are only taking sec-high & sec-critical patches. However, I am leaving the flag for the release owner (Ritu)
Attachment #8666288 - Flags: approval-mozilla-beta?
Attachment #8666288 - Flags: approval-mozilla-beta+
Attachment #8666288 - Flags: approval-mozilla-aurora?
Attachment #8666288 - Flags: approval-mozilla-aurora+
Comment on attachment 8666288 [details] [diff] [review]

As Sylvestre mentioned in comment 13, ESR triage bar allows for sec-crits and sec-highs. As such this bug fix does not meet that bar.
Attachment #8666288 - Flags: approval-mozilla-esr38? → approval-mozilla-esr38-
You need to log in before you can comment on or make changes to this bug.