1790765 - ScriptLoader::MaybeMoveToLoadedList tolerates supposedly impossible state

Status: REOPENED

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

Description

[Jon Coppeard (:jonco)]

This comment in ScriptLoader::MaybeMoveToLoadedList has always bothered me:

  // If it's async, move it to the loaded list.
  // aRequest->GetScriptLoadContext()->mInAsyncList really _should_ be in a
  // list, but the consequences if it's not are bad enough we want to avoid
  // trying to move it if it's not.

https://searchfox.org/mozilla-central/source/dom/script/ScriptLoader.cpp#3633-3636

I'd like to try replacing the check with a release assert so we fail fast rather than covering up a supposedly impossible state.

Comment 1

[Jon Coppeard (:jonco)]

Attachment: Bug 1790765 - Replace superstitious comment in ScriptLoader::MaybeMoveToLoadedList with release assertion r?smaug

An initial try is green with this change.

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7cfadef6f30f
Replace superstitious comment in ScriptLoader::MaybeMoveToLoadedList with release assertion r=smaug

Comment 3

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/7cfadef6f30f

Comment 4

[Andreea Pavel [:apavel]]

Backed out for causing bug 1791309

Backout: https://hg.mozilla.org/integration/autoland/rev/a5b82b696affc2154d22fefe2e016fb116a0ad20

Comment 5

[Andreea Pavel [:apavel]]

Backed out from beta as well https://hg.mozilla.org/releases/mozilla-beta/rev/ab63d31d0d7fa7abf3337c7d1af90436d6b86ea6

Comment 6

[Iulian Moraru]

Backout merged to central: https://hg.mozilla.org/mozilla-central/rev/a5b82b696aff

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:jonco, could you have a look please?
If you still have some work to do, you can add an action "Plan Changes" in Phabricator.
For more information, please visit auto_nag documentation.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

this was backed out.