1842798 - Stop using MozPromise in the module loader

Status: RESOLVED FIXED

(Core :: JavaScript Engine, task, P3)

Description

[Jon Coppeard (:jonco)]

There are a few bugs open at the moment concerning memory leaks that look like they might be caused by the module loader. Bug 1811635 is one example, in which mccr8 says:

I'm not sure exactly how the promise ownership chains work, but I'm guessing the basic issue is that mFetchingModules is not being cleared as expected, and the MozPromise can't be cycle collected because it is some fancy threadsafe thing.

MozPromise is indeed not cycle collected. I think the idea is that promises are transient and so this doesn't matter but it is plausible that this is involved somehow.

I originally used MozPromise in the module loader because I thought it would simplify the code and offload a tricky part of the implementation. However this wasn't entirely successful as it's not clear exactly what resolving/rejecting a promise is going to do and it still ended up pretty hard to understand.

Overall I don't think its use adds much to the module loader and therefore I'd like to remove it.

Comment 1

[Jon Coppeard (:jonco)]

Attachment: Bug 1842798 - Part 1: Remove use of MozPromise to wait for module fetch r?smaug

This replaces the promise with a list of module requests that are waiting for a
resource to be fetched. When the fetch completes the waiting requests are
resumed appropriately. Like MozPromise, this happens via runnables dispatched
to the module loader's event target.

Comment 2

[Jon Coppeard (:jonco)]

Attachment: Bug 1842798 - Part 2: Remove use of MozPromise to wait for module imports to load r?smaug

This replaces the use of a promise to waits for all imports of a module to load
with a counter of reminaing imports in the parent and a pointer to the parent
that is waiting in the child.

Depends on D183272

Comment 3

[Jon Coppeard (:jonco)]

Attachment: Bug 1842798 - Part 3: Remove unused event target from ModuleLoaderBase r?smaug

This was previously used by MozPromise but is no longer required.

Depends on D183273

Comment 4

[Jon Coppeard (:jonco)]

Attachment: Bug 1842798 - Part 4: Removed unused sync event target r?smaug

This is no longer necessary since the module loader is now always synchronous.

Depends on D183586

Comment 5

[Jon Coppeard (:jonco)]

Attachment: Bug 1842798 - Part 4: Update test expectations r?smaug

This fixes two failures that are currently marked as expected to fail.

Depends on D183588

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b2e6bdd781ac
Part 1: Remove use of MozPromise to wait for module fetch r=smaug
https://hg.mozilla.org/integration/autoland/rev/1b89736a463f
Part 2: Remove use of MozPromise to wait for module imports to load r=smaug
https://hg.mozilla.org/integration/autoland/rev/946e4dfe5452
Part 3: Remove unused event target from ModuleLoaderBase r=smaug
https://hg.mozilla.org/integration/autoland/rev/23d10eb8f853
Part 4: Removed unused sync event target r=smaug
https://hg.mozilla.org/integration/autoland/rev/5dad4aeacd53
Part 5: Update test expectations r=smaug
https://hg.mozilla.org/integration/autoland/rev/38669a2ddad9
apply code formatting via Lando

Comment 7

[Natalia Csoregi [:nataliaCs]]

Backed out for causing failures on js/src/tests/test262/language/*

• backout: https://hg.mozilla.org/integration/autoland/rev/af904f365ca830513f5b003eb6366a360b90365c
• push: https://treeherder.mozilla.org/jobs?repo=autoland&selectedTaskRun=UzU4CA5BRr6SPNBzExjfZg.0&revision=38669a2ddad9d4f3a114933fcf73a6c7438d576c
• failure log: https://treeherder.mozilla.org/logviewer?job_id=422889231&repo=autoland&lineNumber=37242

[task 2023-07-17T17:43:16.717Z] 17:43:16     INFO - REFTEST TEST-START | js/src/tests/test262/language/expressions/dynamic-import/eval-export-dflt-cls-anon.js;module;async
[task 2023-07-17T17:43:16.719Z] 17:43:16     INFO - REFTEST TEST-LOAD | file:///builds/worker/workspace/build/tests/jsreftest/tests/js/src/tests/jsreftest.html?test=test262/language/expressions/dynamic-import/eval-export-dflt-cls-anon.js;module;async | 7964 / 11170 (71%)
[task 2023-07-17T17:43:16.772Z] 17:43:16     INFO - TEST-INFO |  FAILED!  InternalError: module record has unexpected status: Evaluating
[task 2023-07-17T17:43:16.883Z] 17:43:16     INFO - REFTEST TEST-UNEXPECTED-FAIL | js/src/tests/test262/language/expressions/dynamic-import/eval-export-dflt-cls-anon.js;module;async | No test results reported. (SCRIPT)
[task 2023-07-17T17:43:16.884Z] 17:43:16     INFO - 
[task 2023-07-17T17:43:16.885Z] 17:43:16     INFO - REFTEST TEST-END | js/src/tests/test262/language/expressions/dynamic-import/eval-export-dflt-cls-anon.js;module;async

Comment 8

[Jon Coppeard (:jonco)]

Turns out my try pushes didn't run all JS reftests.

Comment 9

[Jon Coppeard (:jonco)]

Attachment: Bug 1842798 - Part 3: Complete dynamic imports asynchronously with a microtask r?baku

According to spec this should happen asynchronously. If it doesn't it's
possible for the importing module to still be in the evaluating state when
trying to instantiate and evaluate the dynamically imported module which breaks
the module loader invariants.

Depends on D183273

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/98a1aa86513d
Part 1: Remove use of MozPromise to wait for module fetch r=smaug
https://hg.mozilla.org/integration/autoland/rev/0864a23409e6
Part 2: Remove use of MozPromise to wait for module imports to load r=smaug
https://hg.mozilla.org/integration/autoland/rev/05bcaff61b68
Part 3: Complete dynamic imports asynchronously with a microtask r=smaug
https://hg.mozilla.org/integration/autoland/rev/b153ebf104ef
Part 4: Update test expectations r=smaug

Comment 11

[Sandor Molnar[:smolnar]]

Backed out for causing wpt failures in /html/webappapis/dynamic-markup-insertion/document-write/module-dynamic-import.html

Backout link: https://hg.mozilla.org/integration/autoland/rev/99efb2502534907124c554f48f5cea2fb4518d02

Push with failures

Failure log -> TEST-UNEXPECTED-FAIL | /html/webappapis/dynamic-markup-insertion/document-write/module-dynamic-import.html | document.write in an imported module - assert_true: onload must be called expected true got false

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3528d70acb5f
Part 1: Remove use of MozPromise to wait for module fetch r=smaug
https://hg.mozilla.org/integration/autoland/rev/507da9d36f91
Part 2: Remove use of MozPromise to wait for module imports to load r=smaug
https://hg.mozilla.org/integration/autoland/rev/2a6b3ff79a01
Part 3: Complete dynamic imports asynchronously with a microtask r=smaug
https://hg.mozilla.org/integration/autoland/rev/879154594bb1
Part 4: Update test expectations r=smaug

Comment 13

[Narcis Beleuzu [:NarcisB]]

Backed out for wpt failures on module-tla-import.html

Backout link: https://hg.mozilla.org/integration/autoland/rev/4f30f034e751998477f7830b9f059374003c324c
Log link: https://treeherder.mozilla.org/logviewer?job_id=424674336&repo=autoland&lineNumber=9336

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1642f8c1f603
Part 1: Remove use of MozPromise to wait for module fetch r=smaug
https://hg.mozilla.org/integration/autoland/rev/7b867fa107f5
Part 2: Remove use of MozPromise to wait for module imports to load r=smaug
https://hg.mozilla.org/integration/autoland/rev/6aaab5ae37f2
Part 3: Complete dynamic imports asynchronously with a microtask r=smaug
https://hg.mozilla.org/integration/autoland/rev/ce70368fe145
Part 4: Update test expectations r=smaug

Comment 15

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/1642f8c1f603
https://hg.mozilla.org/mozilla-central/rev/7b867fa107f5
https://hg.mozilla.org/mozilla-central/rev/6aaab5ae37f2
https://hg.mozilla.org/mozilla-central/rev/ce70368fe145