Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Null script block in "fetch an inline module script graph" is unreachable #9864

Closed
Lubrsi opened this issue Oct 18, 2023 · 6 comments · Fixed by #10272
Closed

Null script block in "fetch an inline module script graph" is unreachable #9864

Lubrsi opened this issue Oct 18, 2023 · 6 comments · Fixed by #10272
Labels
clarification Standard could be clearer topic: script

Comments

@Lubrsi
Copy link

Lubrsi commented Oct 18, 2023

What is the issue with the HTML Standard?

In fetch an inline module script graph, there is a null check for script which synchronously calls onComplete with null:

  1. Let script be the result of creating a JavaScript module script using sourceText, settingsObject, baseURL, and options.
  2. If script is null, run onComplete given null, and return.

However, in create a JavaScript module script, none of the return statements return a null script:

  • 8.2
  • 9.1.3
  • 9.3.2
  • 9.5.3
  • 11

Additionally, calling onComplete synchronously is incorrect, as it will run the callback before the ready callbacks are setup later in prepare the script element, causing the load event to be delayed forever.

@domenic
Copy link
Member

domenic commented Apr 11, 2024

Additionally, calling onComplete synchronously is incorrect, as it will run the callback before the ready callbacks are setup later in prepare the script element, causing the load event to be delayed forever.

The problem is not the load event being delayed. "mark as ready" is still called, and that clears the delaying load event flag.

The problem is that "mark as ready" is called (in step 32 > "module" > 3.1) before "steps to run when the result is ready" are set up (in step 33.2.3 or 33.3.3 or 33.4.2). This means that no "error" event will fire for these parse errors.

There are two ways to fix this:

  1. Reorder the spec so that "steps to run when the result is ready" are correctly set up before "mark as ready". This is annoying to do because it will break the if/else if/else structure of steps 31-34.

  2. Delay running "mark as ready" by queuing a microtask. This seems most consistent, because in the success case of "fetch the descendants of and link" we get an immediately-resolved promise from LoadRequestedModules(), and call onComplete from its fulfillment.

I'll work on a spec change to do (2).

@domenic
Copy link
Member

domenic commented Apr 12, 2024

Continuing from web-platform-tests/wpt#45660 (comment). Let's read the HTML and ES specs further to find out how synchronous or asynchronous we should be for:

  • Signaling parse errors
  • Evaluating scripts
  • Signaling evaluation errors or load events

Our main focus is on no-import module scripts. Obviously if a module script has imports, then evaluating the script and signaling evaluation errors or load events are forced to be async, delayed by at least a task. But it's not so simple for the no-import case, as we'll see.

We're also focused mainly on module scripts with async="", since without that, the module scripts have defer="" behavior, meaning they'll always be delayed until the end of the body.


First, if there are parse errors, then we don't involve the ES spec. HTML gets to decide how to handle these.

  • The current spec attempts to signal them synchronously, but because of the spec-factoring issue mentioned in Null script block in "fetch an inline module script graph" is unreachable #9864 they accidentally get swallowed. That's a clear bug. Note that Gecko signals them synchronously.

  • The proposed spec in f1a7962 delays them by a microtask, to be consistent with the no-imports evaluation error case. A bit more below.

  • WebKit / Chromium delay them by a task.

OK. So let's say we have no parse errors. Then, during script preparation, HTML synchronously calls into ECMAScript's Evaluate(), which in turn ends up in InnerModuleEvaluation. This has a special case for zero-import modules, which synchronously calls ExecuteModule(), i.e. runs the code. But if there are imports, then of course we have to wait to fetch them via the host.

After evaluating, the ES spec generates a promise. That promise is given to the host, and has to take at least a microtask to settle. So the host can only react to the execution after a microtask. So this means that if there are evaluation errors (e.g. callAnUndefinedFunction()) we need to wait at least a microtask to fire the error event.

So this means our choices for no-import module scripts are restricted to the following:

  • Script evaluation
    • Per the JS spec, must be synchronous, at least once we enter the JS spec. We could insert a synthetic task to delay "execute the script element" entirely.
    • Gecko does this synchronously.
    • WebKit / Chromium delay evaluation by a task.
  • Evaluation error signals:
    • Per spec, must be delayed by at least a microtask. We could choose to delay them by a task.
    • Gecko signals them synchronously. It is not really possible to make this spec-compliant without layering violations (e.g. monkey-patching the JS spec).
    • WebKit / Chromium delay them by a task.
  • Evaluation complete signals (i.e. load) event: I haven't investigated in detail but I think it's basically the same as evaluation error signals (error event).

Given this information, I guess the cleanest thing is to delay "execute the script element" by a task.

This will match WebKit / Chromium behavior, and ensure that everything is always delayed by a task: no matter whether it's a parse error vs. evaluation error vs. successful evaluation, or whether it's no-import vs. some-imports.

I'll work on updating the spec and tests.

@annevk
Copy link
Member

annevk commented Apr 12, 2024

Thanks you for that comment, that's extremely helpful and clear.

I think I agree with your conclusion. Adding @smaug---- @ljharb @littledan @domfarolino for visibility.

@domfarolino
Copy link
Member

Gecko signals them synchronously. It is not really possible to make this spec-compliant without layering violations (e.g. monkey-patching the JS spec).

You mean because ES currently generates a promise to hand to the host, putting the scheduling (from the host perspective) at a microtask delay at baseline?

  • Evaluation error signals:
    • WebKit / Chromium delay them by a task.

Just for clarity, is this a single task period, when considering everything from script evaluation to error signals observation? Or is this a single task after the post-evaluation microtask is run? Basically I'm asking if from the time to execution --> error signal observation is (a) 1 microtask + 1 full task, or (b) just 1 full task (with no browser implementing the microtask thing).

Eh, hopefully those questions make sense!

@ljharb
Copy link
Contributor

ljharb commented Apr 12, 2024

I agree with @domenic’s thorough analysis.

@domenic
Copy link
Member

domenic commented Apr 13, 2024

You mean because ES currently generates a promise to hand to the host, putting the scheduling (from the host perspective) at a microtask delay at baseline?

Yes.

Just for clarity, is this a single task period, when considering everything from script evaluation to error signals observation? Or is this a single task after the post-evaluation microtask is run? Basically I'm asking if from the time to execution --> error signal observation is (a) 1 microtask + 1 full task, or (b) just 1 full task (with no browser implementing the microtask thing).

I'm not able to distinguish with black-box testing, and I didn't dig into the code to find out.

domenic added a commit to web-platform-tests/wpt that referenced this issue Apr 15, 2024
whatwg/html#9864 discusses the issue. whatwg/html#10272 matches what is tested here.
domenic added a commit that referenced this issue Apr 15, 2024
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Apr 19, 2024
…er, a=testonly

Automatic update from web-platform-tests
Test inserted async script execution order

whatwg/html#9864 discusses the issue. whatwg/html#10272 matches what is tested here.
--

wpt-commits: 5adce6fa88ba7bcb5c119d62736fb30cf1273306
wpt-pr: 45660
ErichDonGubler pushed a commit to ErichDonGubler/firefox that referenced this issue Apr 22, 2024
…er, a=testonly

Automatic update from web-platform-tests
Test inserted async script execution order

whatwg/html#9864 discusses the issue. whatwg/html#10272 matches what is tested here.
--

wpt-commits: 5adce6fa88ba7bcb5c119d62736fb30cf1273306
wpt-pr: 45660
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clarification Standard could be clearer topic: script
Development

Successfully merging a pull request may close this issue.

6 participants