Status

()

defect
P3
normal
RESOLVED FIXED
2 years ago
Last year

People

(Reporter: jonco, Assigned: jonco)

Tracking

unspecified
mozilla60
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox60 fixed)

Details

Attachments

(2 attachments, 3 obsolete attachments)

Assignee

Description

2 years ago
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

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

Updated

2 years ago
Depends on: 1430635
Assignee

Comment 3

Last year
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
Assignee

Comment 4

Last year
Posted patch bug1342416-preload-modules (obsolete) — Splinter Review
WIP patch to implement preloading.
Assignee

Comment 5

Last year
Attachment #8948405 - Attachment is obsolete: true
Assignee

Comment 6

Last year
Posted patch bug1342416-preload-modules v2 (obsolete) — Splinter Review
Attachment #8948406 - Attachment is obsolete: true
Assignee

Comment 7

Last year
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.
Assignee

Comment 8

Last year
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)
Assignee

Comment 9

Last year
(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)
Assignee

Comment 10

Last year
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)
Assignee

Comment 11

Last year
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+
Assignee

Comment 13

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

Comment 14

Last year
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

Last year
bugherder
https://hg.mozilla.org/mozilla-central/rev/77f86d6cf76f
https://hg.mozilla.org/mozilla-central/rev/2da5a0266268
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Assignee

Updated

Last year
Depends on: 1440269
You need to log in before you can comment on or make changes to this bug.