1342416 - Preload module scripts

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P3)

Description

[Jon Coppeard (:jonco)]

Preloading only works with classic scripts at the moment but can be extended to apply to module scripts too.

Comment 2

[Anne (:annevk)]

See also bug 1425310. I suspect most module preloading to happen through that new value.

Comment 3

[Jon Coppeard (:jonco)]

Attachment: bug1342416-set-script-element-api

Add a JS API to allow the element and element attribute associated with a script to be set after the script is created, rather than when it is compile.  This is necesary becuase we want to be able to parse preloaded modules before this information is available.

Comment 4

[Jon Coppeard (:jonco)]

Attachment: bug1342416-preload-modules

WIP patch to implement preloading.

Comment 5

[Jon Coppeard (:jonco)]

Attachment: bug1342416-set-script-element-api v2

Comment 6

[Jon Coppeard (:jonco)]

Attachment: bug1342416-preload-modules v2

Comment 7

[Jon Coppeard (:jonco)]

The above patches are green on try except for a single WPT failure:

https://searchfox.org/mozilla-central/source/testing/web-platform/tests/html/semantics/scripting-1/the-script-element/module/instantiation-error-8.html

This test uses a promise_test() to add an event listener for window error events.  With these changes the event fires before the listener is set up and the test times out.

It's not clear to me whether this is a problem with my changes or a problem with the test.  If I rewrite the test so that it doesn't use promise_test() then it passes.  But it's entirely possible I broke something because I don't really understand the scheduling here that well.

Comment 8

[Jon Coppeard (:jonco)]

As far as I can see, the function passed to proimse_test() is executed in the then callback for the first promise in the promise tests queue:

https://searchfox.org/mozilla-central/source/dom/imptests/testharness.js#518

So that should be executed in a microtask after the inline script has executed I think.  With my changes this happens after the  module script is executed (unsuccessfully, firing the error event).  There is definitely a microtask checkpoint before the script execution though, so I don't see how this is possible:

https://searchfox.org/mozilla-central/source/dom/script/ScriptLoader.cpp#1906

Current sequence of events:
    start inline script
    end inline script
    promise_test argument executes
    module script executes: SyntaxError: import not found: default instantiation-error-1.js:1:7
    ...

Sequence of events with patch applied:
    start inline script
    end inline script
    module script executes: SyntaxError: import not found: default instantiation-error-1.js:1:7
    promise_test argument executes

baku, do you have any ideas what's going on here or how to track this down?

Comment 9

[Jon Coppeard (:jonco)]

(In reply to Jon Coppeard (:jonco) from comment #8)
TIL that our promises don't use microtasks (bug 1193394), which would explain why this test fails.

Comment 10

[Jon Coppeard (:jonco)]

Comment on attachment 8949361 [details] [diff] [review]
bug1342416-set-script-element-api v2

Patch to add a way to associate scripts with DOM element / attribute names after compilation.  This is necessary for preloaded modules, which are compiled before this information is known.

Comment 11

[Jon Coppeard (:jonco)]

Attachment: bug1342416-preload-modules v3

Patch to extend preloading to module scripts.  This mainly extends the current preload mechanism to work for module scripts too.

Module scripts no longer get their source element associated with them when they are compiled but the first time they are run.  That's because we don't have this information when we compile them in the preload case.

Another problem that showed up is that we mustn't put a module load request into the module map until we successfully start fetching it, or we'll end up waiting forever if we try to reload a failed module.

This patch causes the following test to fail (timeout):

html/semantics/scripting-1/the-script-element/module/instantiation-error-8.html

This is because of a problem with our current promise implementation (see previous comment).  This patch disables the test for now.

Comment 12

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8949361 [details] [diff] [review]
bug1342416-set-script-element-api v2

Review of attachment 8949361 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsapi.cpp
@@ +4676,5 @@
>                             chars.get(), length, fun);
>  }
>  
> +JS_PUBLIC_API(bool)
> +JS::SetScriptSourceElement(JSContext* cx, HandleScript script,

nit: Should this function include a comment such as: "Should be called on a give script before the first execution" ?  And identically for all other function added by this patch, in which case, should these function be named "init" instead?

::: js/src/jsscript.cpp
@@ +1491,5 @@
>  }
>  
>  /* static */ bool
> +ScriptSourceObject::setElementProperties(JSContext* cx, HandleScriptSource source,
> +                                         HandleObject element, HandleString elementAttrName)

Q: initElementProperties ?

Comment 13

[Jon Coppeard (:jonco)]

(In reply to Nicolas B. Pierron [:nbp] from comment #12)
Good points, I'll rename to 'init' and add a comment.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/77f86d6cf76f
Add JS API to associate scripts with DOM elements after compilation r=nbp
https://hg.mozilla.org/integration/mozilla-inbound/rev/2da5a0266268
Preload module scripts r=baku

Comment 15

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/77f86d6cf76f
https://hg.mozilla.org/mozilla-central/rev/2da5a0266268