Preload module scripts

RESOLVED FIXED in Firefox 60

Status

()

P3
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: jonco, Assigned: jonco)

Tracking

unspecified
mozilla60
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox60 fixed)

Details

Attachments

(2 attachments, 3 obsolete attachments)

Preloading only works with classic scripts at the moment but can be extended to apply to module scripts too.
Priority: -- → P3

Updated

2 years ago
Duplicate of this bug: 1379945

Comment 2

a year ago
See also bug 1425310. I suspect most module preloading to happen through that new value.
Depends on: 1430635
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?
Flags: needinfo?(amarchesini)
(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.
Flags: needinfo?(amarchesini)
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.
Attachment #8949362 - Attachment is obsolete: true
Attachment #8950301 - Flags: review?(amarchesini)
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.

Comment 14

a year ago
Pushed by jcoppeard@mozilla.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

Comment 15

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/77f86d6cf76f
https://hg.mozilla.org/mozilla-central/rev/2da5a0266268
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Depends on: 1440269
You need to log in before you can comment on or make changes to this bug.