There is no active script when a user triggers an inline event handler

[domenic]

Consider <div onclick="import('./foo.js')">. Per spec this code is evaluated via https://html.spec.whatwg.org/multipage/webappapis.html#the-event-handler-processing-algorithm. There is no active script at this time, causing the import() spec to blow up (fail an assert).

This is somewhat similar to what was fixed by #3163. The fix is less obvious though. I guess we'd create a synthetic script representing the page's inline event handlers??

/cc @whatwg/modules

[annevk]

I suspect this is also of interest to @bzbarsky.

[bzbarsky]

Hmm.... https://html.spec.whatwg.org/multipage/webappapis.html#the-event-handler-processing-algorithm calls https://heycam.github.io/webidl/#invoke-a-callback-function which calls https://tc39.github.io/ecma262/#sec-call

In this case, that lands in https://tc39.github.io/ecma262/#sec-ecmascript-function-objects-call-thisargument-argumentslist and https://tc39.github.io/ecma262/#sec-prepareforordinarycall and that will set things up with the [[ScriptOrModule]] of the function being called. That comes from https://html.spec.whatwg.org/multipage/webappapis.html#getting-the-current-value-of-the-event-handler which does https://tc39.github.io/ecma262/#sec-functioncreate which calls https://tc39.github.io/ecma262/#sec-functioninitialize which relies on GetActiveScriptOrModule at that point.

Which may well not be defined (I got a bit lost while digging)? But then I don't quite follow the assert added in #3163 which claims that jobs are only enqueued while a script is active; we can certainly enqueue jobs inside an event handler function, right?

Of course there's the extra fun of what happens with div.onclick = import.bind(stuff) which really doesn't have a scripted function anywhere around.

I've sort of lost track of how the ES spec uses "script or module" bits and why it thinks they should always exist, but it seems like fundamentally the issue is that ES thinks the only way you can start a new execution is by "running a script or evaluating a module", while in reality you can also start by calling a (possibly built-in) function directly. This seems like a pretty bad semantic mismatch between the ES spec and the web. :(

[domenic]

Which may well not be defined (I got a bit lost while digging)?

So, I guess the case is that if the page author has ever triggered the getter (e.g. just done el.onclick;), then the function is created at a time where an active script is present. But, if they never do, then this presumably happens in response to the user clicking, when there is no active script present.

But then I don't quite follow the assert added in #3163 which claims that jobs are only enqueued while a script is active; we can certainly enqueue jobs inside an event handler function, right?

Yeah, it seems like that's broken here too, as a result of this particular case.

Of course there's the extra fun of what happens with div.onclick = import.bind(stuff) which really doesn't have a scripted function anywhere around.

import() is more like super() in being a syntactic form, not a function, so this won't work. But your point stands, with e.g. eval.bind(undefined, "import(...)").

I've sort of lost track of how the ES spec uses "script or module" bits and why it thinks they should always exist

So, it's basically only to support import(). When you do import(), how do we resolve module specifiers? That's the only real consumer. Right now, the import() spec assumes (step 2) that GetActiveScriptOrModule is not null.

I think we have two paths:

• Try to plug these holes, so that there's always an active script or module.
• Create a fallback (besides asserting). Example:
  • import() throws a TypeError
  • import() resolves relative to the current settings object's API base URL

I am leaning toward the API base URL idea.

This would be observably equivalent to trying to plug the hole by creating some synthetic script for event handler purposes, but would also catch any other potential places we missed, and would be import()-specific.

ES thinks the only way you can start a new execution is by "running a script or evaluating a module", while in reality you can also start by calling a (possibly built-in) function directly. This seems like a pretty bad semantic mismatch between the ES spec and the web. :(

This is all relatively recent, so I wouldn't say this is a big fundamental problem; we can fix it.

I'd be curious from an implementer point of view which solution makes the most sense, between trying to maintain the invariant via synthetic scripts, versus using a fallback. For example, I could imagine that implementers might like the synthetic script approach, because it would help things like stack traces, the error event, and debuggers, which in their implementation probably also have a concept of "script". (And in the case of the error event, this correspondence kinda-sorta exists in the spec.)

[bzbarsky]

Ah, if this is only relevant for import, that makes things a bit simpler...

So normally the url of the script is not used as a base url for things in the web platform. I agree that this is a little silly in various cases but especially for imports.

Falling back to the current settings object API base URL makes a lot of sense to me. That will ensure that things are always defined, and in the inline event handler case definitely produce the "right" behavior (use the URL of the page as the base URL).

This is all relatively recent

Gotcha. I agree that makes life simpler.

In terms of implementation, I'm not sure what works best. In SpiderMonkey, a non-builtin function always has an associated "script" which has a "filename" (typically a URL, but it's just an opaque string as far as SpiderMonkey is concerned, for the most part). For inline event handlers that we compile from a an attr value string, we set that "filename" to the document URL of the element's owner document at the moment when compilation happens.

[domfarolino]

import() resolves relative to the current settings object's API base URL

I'm personally a fan of this. In Chrome right now (< this link is outdated) we fallback to the document base URL, which I believe in this case, is equivalent to using the current settings object's API base URL.

The more I look around..could someone explain the difference between current settings object and entry settings object?

Are we thinking the fix here is to make the import spec not Assert so harshly, and then in 8.1.3.8.2 just check to see if referencingScriptOrModule is null, and if so:

• Amend step 2.1 to use the current settings object's [API base URL] to resolve a module specifier
• Amend step 3 to setup some sort of default options (maybe default classic script fetch options)
• (among maybe some other things)

...or do I have this wrong?

[annevk]

@domfarolino see the example in https://html.spec.whatwg.org/multipage/webappapis.html#realms-settings-objects-global-objects for an illustration of the difference between entry/incumbent/current/relevant (settings objects are 1:1 with Realms).

[domenic]

Are we thinking the fix here is

Yeah that's basically it. I think I would change "resolve a module specifier" to take a base URL instead of a script. Then we'd add a steps after step 1 that create baseURL and fetchOptions variables.

Also, I found a bug: step 4 just says "settings object" when it should probably say "referencing script's settings object". Or, in this fixed universe, we'd also create a settingsObject variable.

Are you, uh, volunteering? :)

[bzbarsky]

In Chrome right now we fallback to the document base URL, which I believe in this case, is equivalent to using the current settings object's API base URL.

I haven't looked into how exactly what your fallback code looks like, but use of "the document" is a red flag, because there are multiple documents around. Most simply, if you get the event handler string compiled while the node is in one document, then you adopt it into another one, now the current settings is whatever the document was where you compiled but "the document" is probably where you adopted into. Which one does Chrome actually use?

I looked into what Gecko does here. In Gecko, if you <div onclick="import(stuff)"> you get an exception:

SyntaxError: import declarations may only appear at top level of a module

so presumably we only allow import() at toplevel of a <script type="module"> or a module imported from such. Not sure whether that's a restriction that used to be in the spec but was then loosened?

[domenic]

The issue is that Gecko doesn't implement the import("x") expression syntax at all, so it thinks you're trying to do an import "x" statement, and gets confused.

[domfarolino]

Are you, uh, volunteering? :)

Indeed! I'd like to take a look at this this weekend, I should have more time then.

Which one does Chrome actually use?

Ahh, I'm not sure actually. I'll dig into this a bit too but can't guarantee a timely answer to that one.

[domfarolino]

Ok @bzbarsky to answer your question, in Chrome right now we use the base URL of the document the handler was first compiled/ran in. For example, if:

• An event handler that contains import(...) is compiled/run in an iframe
• The node with said event handler is adopted by an outer document
• The node's event handler is evaluated again (onclick for example, we click again) in the outer document

...the "document's base URL" we use in subsequent evaluations of import(...) is that of the document in which the event handler was first compiled/run. Is this up to spec? From what you said it sounds like it is, but personally from what I'm reading it seems to make sense that adoption would change what the "current settings object" is (though atm I'm fairly unfamiliar with Realm records etc).

[domfarolino]

Also, I found a bug: step 4 just says "settings object" when it should probably say "referencing script's settings object"

Hmm, doesn't it say "referencing script's settings object" right now? Also, as you mentioned in the case where there is no referencing script and we'd have to create a settingsObject variable, I'm thinking this could just be the "current settings object", since it is that object that we're pulling the fallback base URL from right? And for "descendent script options" used, we'll probably have to initialize a new script fetch options struct with reasonable defaults since we have nothing to inherit from, or maybe we could define something similar to default classic script fetch options, differing only in the default credentials mode?

[domenic]

...the "document's base URL" we use in subsequent evaluations of import(...) is that of the document in which the event handler was first compiled/run. Is this up to spec?

This doesn't seem quite right. Right now the spec says to "crash" (it tries to get a property of the active script, and the active script is null). After the changes proposed here it will be the base URL of the node document of the attribute's owner element, which can be different from the one that's causing the click.

This follows from https://html.spec.whatwg.org/multipage/webappapis.html#getting-the-current-value-of-the-event-handler step 9/10, which establish that the function is created in the node document of the attribute's owner element.

Hmm, doesn't it say "referencing script's settings object" right now?

Yeah, this got fixed by #3700.

Also, as you mentioned in the case where there is no referencing script and we'd have to create a settingsObject variable, I'm thinking this could just be the "current settings object", since it is that object that we're pulling the fallback base URL from right?

Yeah, that was what I was thinking.

And for "descendent script options" used, we'll probably have to initialize a new script fetch options struct with reasonable defaults since we have nothing to inherit from, or maybe we could define something similar to default classic script fetch options, differing only in the default credentials mode?

I think we can just rename "default classic script fetch options" to "default script fetch options". The credentials mode should stay the same for both; changing that default is tracked by #3656.

[domenic]

@domfarolino interestingly it looks like we'll also need to fix HostResolveImportedModule, not just HostImportModuleDynamically. https://tc39.github.io/proposal-dynamic-import/ calls both of them with the active, and potentially null, script.

[domfarolino]

Good catch, will update that too.

[domenic]

I skipped over an important comment of @bzbarsky's, while working on #4181:

But then I don't quite follow the assert added in #3163 which claims that jobs are only enqueued while a script is active; we can certainly enqueue jobs inside an event handler function, right?

This is still correct. In particular the trouble case is something like

button.setAttribute("onclick", `
  Promise.resolve('import("./example.js").catch(e => print("caught", e))')
       .then(eval);
`);

plus the user actually clicking on the button. In this case there will be no active script when FunctionCreate is called in step 10 of get the current value of the event handler, which means the same is true when .then() calls EnqueueJob, which breaks the assert.

I will update #4181 to remove the assert and guard the related steps. This is in line with our conclusion that it's OK for there to not be an active script in these pathological cases, and instead have import() fall back to using the document's base URL.