Bug 1740534 Comment 17 Edit History

Note: The actual edited comment in the bug view page will always show the original commenter’s name and original timestamp.

I'm categorizing this as sec-moderate from https://wiki.mozilla.org/Security_Severity_Ratings/Client with a rationale of:
- I don't believe this is directly exploitable.  I also don't believe this is indirectly exploitable.
  - This only allows a single draining of the microtask queue after a task where content was already running.
  - We will not have run any shutdown-related GC's or any other specialized shutdown logic on the way out of the primary runnable because we have not yet begun to unwind that loop.
    - The scope and JS context are all still valid.  The dying global is a DOM construct, not a JS construct.
  - WorkerRefs will correctly fail to be created at this time which code should handle.
- However this is still definitely an invariant violation and there's no need to point people directly at it, so this still gets to be a security bug.
- Because there is an invariant being violated here, as in this bug, code can get upset as its invariants get violated as code runs after WorkerRefs had their callbacks invoked and their DETHs disconnected.  So we do expect assertion violations and potentially nullptr dereferences.
  - DOMEventTargetHelper instance will bind to parents even though CheckCurrentGlobalCorrectness would return false, however, the DETH instances will properly be detached again [on our way out of the primary run loop](https://searchfox.org/mozilla-central/rev/4ca2f73ae9346709d39de2c8fe33874e4203b9e6/dom/workers/RuntimeService.cpp#2245) (and actually would be done again in nsIGlobalObject's destructor where the scope closes in the next line, but no new DETHs should be added during that interval; if they are, that would be a different, serious bug).

I think it's worth fixing and landing this bug now with a target of uplifting into beta here early in the cycle.  It could also make sense to let this be uplifted to release if there are dot releases but I don't believe this would necessarily be urgent for security reasons, but could be desirable as there could reasonably be non-exploitable crashes that could be eliminated.
I'm categorizing this as sec-moderate from https://wiki.mozilla.org/Security_Severity_Ratings/Client with a rationale of:
- I don't believe this is directly exploitable.  I also don't believe this is indirectly exploitable.
  - This only allows a single draining of the microtask queue after a task where content was already running.
  - We will not have run any shutdown-related GC's or any other specialized shutdown logic on the way out of the primary runnable because we have not yet begun to unwind that loop.
    - The scope and JS context are all still valid.  The dying global is a DOM construct, not a JS construct.
  - WorkerRefs will correctly fail to be created at this time which code should handle.
- However this is still definitely an invariant violation and there's no need to point people directly at it, so this still gets to be a security bug.  (It's of course plausible that this could be chained with other, more serious bugs.)
- Because there is an invariant being violated here, as in this bug, code can get upset as its invariants get violated as code runs after WorkerRefs had their callbacks invoked and their DETHs disconnected.  So we do expect assertion violations and potentially nullptr dereferences.
  - DOMEventTargetHelper instance will bind to parents even though CheckCurrentGlobalCorrectness would return false, however, the DETH instances will properly be detached again [on our way out of the primary run loop](https://searchfox.org/mozilla-central/rev/4ca2f73ae9346709d39de2c8fe33874e4203b9e6/dom/workers/RuntimeService.cpp#2245) (and actually would be done again in nsIGlobalObject's destructor where the scope closes in the next line, but no new DETHs should be added during that interval; if they are, that would be a different, serious bug).

I think it's worth fixing and landing this bug now with a target of uplifting into beta here early in the cycle, especially as it is likely that this will likely help eliminate a number of fuzzing errors that could otherwise be distracting.  It could also make sense to let this be uplifted to release if there are dot releases but I don't believe this would necessarily be urgent for security reasons, but could be desirable as there could reasonably be non-exploitable crashes that could be eliminated.

Back to Bug 1740534 Comment 17