Closed Bug 1779421 Opened 3 months ago Closed 2 months ago

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

Categories

(Core :: JavaScript Engine, task, P1)

task

Tracking

()

RESOLVED FIXED
105 Branch
Tracking Status
firefox105 --- fixed

People

(Reporter: jonco, Assigned: jonco)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

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.

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.

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

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

Depends on D151834

Severity: -- → N/A
Status: NEW → ASSIGNED
Priority: -- → P1
Pushed by jcoppeard@mozilla.com:
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
Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → 105 Branch
Regressions: 1787926
You need to log in before you can comment on or make changes to this bug.