1779421 - Leave module's AsyncEvaluation field set to true after evaluation as per latest spec

Status: RESOLVED FIXED

(Core :: JavaScript Engine, task, P1)

Description

[Jon Coppeard (:jonco)]

Following on from bug 1779247. In earlier versions of the top level await spec, this field was set to false after evaluation finished. That is no longer the case.

We currently use setting this to false as a place to potentially reset our moduleAsyncEvaluatingPostOrder counter, so that will have to be reworked.

Comment 1

[Jon Coppeard (:jonco)]

Attachment: Bug 1779421 - Part 1: Rework module AsyncEvaluation field to match the spec r?yulia

As per the lastest spec this now stays true after evaluation.

This patch keeps a very similar implemenatation and resets the field after
evaluation, except that we now report this special value as true rather than
false. We assert if we try and use get the sort order after this has happened.

We also reset the runtime counter in the same way as before.

Comment 2

[Jon Coppeard (:jonco)]

Attachment: Bug 1779421 - Part 2: Fix reset of post order counter r?yulia

Adding tests (see next patch) revealed two issues with the async evaluation
post order counter.

The first is an off-by-one error in the reset condition which is easily fixed.

The second I'm less sure of. I added an assertion that the finished post order
was less than the next post order counter and was surprised to find it didn't
hold. This is because although async evaluations start in the order given by
the assigned orginal they may finish in any order. So we may reset the counter
while there are still evaluations outstanding.

V8 uses this same scheme and has a long comment explaining why it works:

https://source.chromium.org/chromium/chromium/src/+/main:v8/src/execution/isolate-inl.h;l=190-213?q=async_evaluating_ordinal&ss=chromium%2Fchromium%2Fsrc

Maybe I don't understand this but I'm not convinced by paragraph 3.

This patch uses a conceptually simpler scheme with a counter of finished
evaluations. We reset when the counter reaches the next post order, which
allows executions to finish in any order. This allows us to add the assertion.

Depends on D151833

Comment 3

[Jon Coppeard (:jonco)]

Attachment: Bug 1779421 - Part 3: Add tests for expected module status and async evaluating state r?yulia

This adds a shell function 'clearModules' to clear all loaded modules, so that
tests can be insulated from one another.

Depends on D151834

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7fa1eac15d43
Part 1: Rework module AsyncEvaluation field to match the spec r=yulia
https://hg.mozilla.org/integration/autoland/rev/d44ac2d93a1c
Part 2: Fix reset of post order counter r=yulia
https://hg.mozilla.org/integration/autoland/rev/c02f1a1d3cf6
Part 3: Add tests for expected module status and async evaluating state r=yulia

Comment 5

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/7fa1eac15d43
https://hg.mozilla.org/mozilla-central/rev/d44ac2d93a1c
https://hg.mozilla.org/mozilla-central/rev/c02f1a1d3cf6