Preloading only works with classic scripts at the moment but can be extended to apply to module scripts too.
See also bug 1425310. I suspect most module preloading to happen through that new value.
Created attachment 8948405 [details] [diff] [review] 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.
Assignee: nobody → jcoppeard
Created attachment 8948406 [details] [diff] [review] bug1342416-preload-modules WIP patch to implement preloading.
Created attachment 8949361 [details] [diff] [review] bug1342416-set-script-element-api v2
Attachment #8948405 - Attachment is obsolete: true
Created attachment 8949362 [details] [diff] [review] bug1342416-preload-modules v2
Attachment #8948406 - Attachment is obsolete: true
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.
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?
(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 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.
Attachment #8949361 - Flags: review?(nicolas.b.pierron)
Created attachment 8950301 [details] [diff] [review] 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 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 ?
Attachment #8949361 - Flags: review?(nicolas.b.pierron) → review+
Attachment #8950301 - Flags: review?(amarchesini) → review+
(In reply to Nicolas B. Pierron [:nbp] from comment #12) Good points, I'll rename to 'init' and add a comment.
Pushed by email@example.com: 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
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.