Closed Bug 1342416 Opened 5 years ago Closed 4 years ago

Preload module scripts


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




Tracking Status
firefox60 --- fixed


(Reporter: jonco, Assigned: jonco)




(2 files, 3 obsolete files)

Preloading only works with classic scripts at the moment but can be extended to apply to module scripts too.
Priority: -- → P3
Duplicate of this bug: 1379945
See also bug 1425310. I suspect most module preloading to happen through that new value.
Depends on: 1430635
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
Attached patch bug1342416-preload-modules (obsolete) — Splinter Review
WIP patch to implement preloading.
Attachment #8948405 - Attachment is obsolete: true
Attached patch bug1342416-preload-modules v2 (obsolete) — Splinter Review
Attachment #8948406 - Attachment is obsolete: true
The above patches are green on try except for a single WPT failure:

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:

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:

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)
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):


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.
Pushed by
Add JS API to associate scripts with DOM elements after compilation r=nbp
Preload module scripts r=baku
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Depends on: 1440269
You need to log in before you can comment on or make changes to this bug.