Legacy test for module script error handling: choice-of-error-1.html, error-type-1.html should be updated

[allstarschh]

While I am implementing whatwg/html#8253 in Firefox, I notice that there are two WPT tests that should be updated

1. choice-of-error-1.html
2. error-type-1.html

These two tests were added in c7a8ddc
and were added to address whatwg/html#2991

But now with whatwg/html#8253 is merged, those two tests are not valid anymore.
Below I explain them in detail:

1. choice-of-error-1.html

With the current spec, if a sub-module script has a parse error

https://html.spec.whatwg.org/#hostloadimportedmodule

Step  14.
  3. Otherwise, if moduleScript's parse error is not null, then:
    1. Let parseError be moduleScript's parse error.
    2. Set completion to ThrowCompletion(parseError).
    3. If loadState is not undefined and loadState.[[ErrorToRethrow]] is null, set loadState.[[ErrorToRethrow]] to parseError
  ..
  5. Perform FinishLoadingImportedModule(referrer, moduleRequest, payload, completion).

https://tc39.es/ecma262/#sec-FinishLoadingImportedModule

Step 2. If payload is a GraphLoadingState Record, then
  a. Perform ContinueModuleLoading(payload, result).

https://tc39.es/ecma262/#sec-ContinueModuleLoading

Step 3. Else,
  a. Set state.[[IsLoading]] to false.
  b. Perform ! Call(state.[[PromiseCapability]].[[Reject]], undefined, « moduleCompletion.[[Value]] »).

https://html.spec.whatwg.org/#fetch-the-descendants-of-and-link-a-module-script

Step 7. Upon rejection of loadingPromise, run the following steps
  1. If state.[[ErrorToRethrow]] is not null, set moduleScript's error to rethrow to state.[[ErrorToRethrow]] and run onComplete given moduleScript.

The parse error (from syntaxerror.js#1c or syntaxerror.js#1d) will be reported to the two module graphs, choice-of-error-1a.js and choice-of-error-1b.js

Hence, the errors should be identical, meaning that the assert_not_equals(log[0], log[2] should be changed to assert_equals(log[0], log[2]

The test worked because of the legacy behavior, find the first parse error
legacy spec link PR 2991
legacy spec link before PR 8253
will report different parse errors on both the module graphs:

1a.js
  1b.js
    1a.js (already in discoveredSet, continue)
    syntaxerror.js#1d (found first parse error)
    
1b.js
  1a.js
    1b.js (already in discoveredSet, continue)
    syntaxerror.js#1c (found first parse error)

2. error-type-1.html
   The module error-type-1.js will import syntaxerror.js and 404.js

And syntaxerror.js has been fetched previously, so the parse error should be reported first.

For syntaxerror.js, the spec link is the same as above.
For the null script of 404.js
https://html.spec.whatwg.org/#hostloadimportedmodule

Step  14.
  2. If moduleScript is null, then set completion to [ThrowCompletion](new TypeError)

https://html.spec.whatwg.org/#fetch-the-descendants-of-and-link-a-module-script

Step 7.2. Otherwise, run onComplete given null.

The test was to address the spec text introduced in whatwg/html#2991

fetch a single module script

Step 8. If any of the following conditions are met,... asynchronously complete this algorithm with null, and abort these steps:
  response's type is "error"

fetch the descendants of a module script

Step 7. For each url in URLs, perform the internal module script graph fetching procedure ...
...
If any of the invocations of the internal module script graph fetching procedure asynchronously complete with null, asynchronously complete this algorithm with null, aborting these steps.

I also quote the spec before whatwg/html#8253
fetch the descendants of a module script

Step 11.2. If result is null, then set failed to true, run onComplete with null, and abort these steps.

Therefore, for error-type-1.html should be updated as well.

I have uploaded my fixes in
fix for choice-of-error-1.html
fix for error-type-1.html

However, according to wpt.fyi
choice-of-error-1.html
error-type-1.html,
landing those fixes will regress? other browser vendors as well,

So I'd like to request a consensus first
CCing @nicolo-ribaudo @domenic

[nicolo-ribaudo]

The new behavior seems to be more consistent with module evaluation error (where an evaluation error effectively propagates to the whole cycle), but by default I'd leant towards restoring the old behavior.

I need more time to actually go through your explanation and understand what's going on here.

I see that Safari is failing those two tests, would it start passing with the proposed test changes?

[nicolo-ribaudo]

Is it correct that the current version of the spec will stop the loading process on first error and report that, while the old version required to keep fetching the rest of the module graph (even if we know that it's in a failure state) to find the appropriate error to throw?

Or is this just about the error object being a fresh one even if it's pointing to the same underlying syntax error?

[allstarschh]

Is it correct that the current version of the spec will stop the loading process on first error and report that,

Yes, the first error reported through FinishLoadingImportedModule in HostLoadImportedModule will reject the promise returned from LoadRequestedModules in fetch the descendants of and link a module script.

while the old version required to keep fetching the rest of the module graph (even if we know that it's in a failure state) to find the appropriate error to throw?

Not entirely correct.

If fetching any of the sub-modules fails, for example, 404, or CSP check fails, the fetch of the module graph aborts immediately.

But if every one of the sub-module is fetched successfully, (even any of them has a parse error), it will wait until all fetch completes.

But the legacy spec will have the 'find the first parse error' algorithm.

See
https://web.archive.org/web/20171030003126/https://html.spec.whatwg.org/#fetch-the-descendants-of-a-module-script
https://web.archive.org/web/20221130023614/https://html.spec.whatwg.org/#fetch-the-descendants-of-a-module-script

Or is this just about the error object being a fresh one even if it's pointing to the same underlying syntax error?

No
You mean the error objects in choice-of-error-1.html?
Before PR 8253, one SyntaxError is from syntaxerror.js?1c, while the other SyntaxError is from syntaxerror.js?1d.
So two error objects here.

After PR 8253, one SyntaxError (could be syntaxerror.js#1c or #1d, depends on which is received first), but the error is reported to the two different module graphs (choice-of-errror-1a.js, choice-of-error-1b.js)

[Gae24]

At Servo we are rewriting our implementation to follow spec and encountered the same regressions.

[nicolo-ribaudo]

Any opinion on what behavior is better, between the current and the previous one?

(trying to figure out if the next step is a spec change or a tests change)

[allstarschh]

I think it depends on how the spec wants to handle the error.

From whatwg/html#2991, it is trying to unifying the error behavior,

import "404.js";
import "syntaxerror.js";

which error should be reported? This is also related to whether the onerror handler should be called or an exception should be thrown when executing.

Also if there are multiple SyntaxErrors in the module graph, which one should be reported, ... etc.

Or, do we need to report all of them?

Current spec reports the 1st error found whatever it is. Whereas the legacy reports the null first, and then traversed to find the depth-first one.