whatwg/html

HostLoadImportedModule missing empty check for requested modules

Closed this issue · 5 comments

shannonbooth commented

What is the issue with the HTML Standard?

10ed38e changed HostLoadImportedModule so that it performs early validation of modules.

Step 7. of HostLoadImportedModule now says:

  1. If referrer is a Cyclic Module Record and moduleRequest is equal to the first element of referrer.[[RequestedModules]], then:

Which seems to indicate that referrer.[[RequestedModules]] will always have at minimum one element. However, for the following HTML:

<script type="module">
import * as m from "./something.js";
</script>

And something.js:

import("./does-not-matter.js");

The import in something.js will have referrer as something.js, and the CyclicModule something.js will have an empty [[RequestedModules]] when it hits HostLoadImportedModule invoked from https://tc39.es/ecma262/#sec-import-call-runtime-semantics-evaluation

domenic commented

Which seems to indicate that referrer.[[RequestedModules]] will always have at minimum one element.

I don't think that's true? If there is no first element of referrer.[[RequestedModules]], then certain moduleRequest will not be equal to it, since it doesn't exist.

  • 👍1
shannonbooth commented

@domenic okay, thanks! Seems like a case of me misunderstanding the specifics / meaning of the prose. From a search, from another point of reference, from checking the HTML parsing algorithms they seem to clarify this type of statement by saying "if any", e.g: https://html.spec.whatwg.org/multipage/parsing.html#foster-parent

Maybe I could make a PR to also add that editorial type change? e.g:

If referrer is a Cyclic Module Record and moduleRequest is equal to the first element of referrer.[[RequestedModules]] (if any), then:

domenic commented

Hmm, I'd be hesitant to introduce that precedent that we need to carefully consider the empty case in all such element references.

  • 👍1
shannonbooth commented

Admittedly, the background of this issue is just that I had misinterpreted these steps at first, and had implemented this in a buggy manner that clearly would result in crashing websites: LadybirdBrowser/ladybird@5af613a (we are missing some support in modules, I am looking at slowly building it out and improve our test situation).

I was still a little confused when the bug was reported and I made the fix: LadybirdBrowser/ladybird#2677 (comment) Until I eventually narrowed down a reproducer from vscode.dev to understand what was going on.

I'm happy to just leave as is if there are wider implications though, and will keep out-of-bounds-ness being a possibility for the empty case when seeing this language in the future.

domenic commented

I'm definitely open to revisiting, if others chime in saying they find the current wording confusing.

  • 👍1