1962477 - Vestigial Forbidding Script Mechanism can be removed

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, task)

Description

[Matthew Gaudet (he/him) [:mgaudet]]

There's an old script forbidding mechanism that no longer seems to have any consumers and can be removed.

Comment 1

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Bug 1962477 - Delete WorkerPrivate::StartForbiddingScript r?asuth

I think this has been dead code since D177511

Comment 2

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Bug 1962477 - Delete vestigial StopForbiddingScript r?asuth

Comment 3

[Andrew Sutherland [:asuth] (he/him)]

Thanks for noticing this and providing patches!

Restating:

• This mechanism was introduced in bug 1545345 to ensure that ClearMainEventQueue would not allow calls into content as part of a set of defense-in-depth changes to address and mitigate an XHR UAF. As captured in this analysis on the bug there was a lot going on, but the key thing as it relates to that bug was that method would use the StartForbiddingScript/StopForbiddingScript to cause all uses of CallbackObject to fail to call into content while we were spinning the event loop to clear all pending runnables.
  • Runnables that were wrapped as WorkerRunnables would check a flag that said we were clearing the event loop and not actually run, but an emergent thing going on was that runnables dispatched from IPC would not be wrapped into WorkerRunnables and would end up running, and in particular BroadcastChannel would end up calling into content.
• The fundamental UAF was due to problems in the worker XHR implementation which were addressed in multiple subsequent passes. This guard was just about not letting JS run in places it should absolutely not be running and where the global was already doomed but we couldn't express that correctly without making major changes (which were subsequently made in https://phabricator.services.mozilla.com/D177511 in bug 1800659). These subsequent XHR fixes in particular ensured that bug can never come back:
  • Bug 1858033 made Proxy::mXMLHttpRequestPrivate a WeakPtr which ensured we couldn't have a stale UAF pointer.
  • Bug 1900681 fixed a double-clocking of the state machine that had likely been the source of most invariant violations.
• As noted in comment 1, https://phabricator.services.mozilla.com/D177511 as part of bug 1800659 eliminated ClearMainEventQueue and let us have a much cleaner lifecycle transition for nsIGlobalObject for when self.close() has been called and control returns to the top-level. (Much of the weirdness in bug 1545345 and the underlying lifecycle management related to the need to wait until nested syncloops were unwound.) Now there is only one place where we will call DisconnectGlobalTeardownObservers when control flow is yielded back to the outermost worker event loop and where we will also call WorkerGlobalScope::NoteTerminating which will idempotently call nsIGlobalObject::StartDying which sets nsIGlobalObject::mIsDying which gets checked by nsIGlobalObject::IsScriptForbidden (and still will after https://phabricator.services.mozilla.com/D246651).
  • Note that in the case Worker.terminate() is invoked by the owner of the worker, it's still the case that the NotifyRunnable WorkerControlRunnable can be invoked with Canceling and cause NoteTerminating to be called when there is a syncloop or JS on the stack, but we won't call nsIGlobalObject::DisconnectGlobalTeardownObservers until we've unwound to the outermost worker event loop which is important for C++ DOM object invariant simplicity. Note that while we do expect an uncatchable exception to be thrown by any JS interrupt that's invoked, bug 1837302 does still need to be addressed for the syncloop/sync event dispatch cases to ensure uncatchable exceptions propagate all the way up the stack.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e38c4ca77dca
Delete WorkerPrivate::StartForbiddingScript r=asuth,dom-worker-reviewers
https://hg.mozilla.org/integration/autoland/rev/4ed65d34d15a
Delete vestigial StopForbiddingScript r=asuth,smaug

Comment 5

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/e38c4ca77dca
https://hg.mozilla.org/mozilla-central/rev/4ed65d34d15a