Closed
Bug 1361369
Opened 7 years ago
Closed 7 years ago
Support async on inline module scripts
Categories
(Core :: JavaScript Engine, enhancement, P3)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla59
Tracking | Status | |
---|---|---|
firefox59 | --- | fixed |
People
(Reporter: jaffathecake, Assigned: jonco)
References
Details
(Keywords: triage-deferred)
Attachments
(6 files, 2 obsolete files)
(deleted),
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jonco
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jonco
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_4) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/58.0.3029.81 Safari/537.36 Steps to reproduce: https://module-script-tests-rgjnxtrtqq.now.sh/async Inline scripts with the async attribute should execute according to the async rules. Actual results: They're executing according to the defer rules. Expected results: Fast scripts should execute before slow scripts.
Updated•7 years ago
|
Component: Untriaged → JavaScript Engine
Product: Firefox → Core
Updated•7 years ago
|
Severity: normal → enhancement
Updated•7 years ago
|
Keywords: triage-deferred
Priority: -- → P3
Assignee | ||
Comment 1•7 years ago
|
||
Do you still have the example to reproduce this? That link doesn't work any more.
Flags: needinfo?(jaffathecake)
Reporter | ||
Comment 2•7 years ago
|
||
Sorry. Updated link: https://module-script-tests-sreyfhwvpq.now.sh/async
Flags: needinfo?(jaffathecake)
Assignee | ||
Comment 3•7 years ago
|
||
I can reproduce this. It seems the async attribute is not propagated to module imports.
Assignee: nobody → jcoppeard
Assignee | ||
Updated•7 years ago
|
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Assignee | ||
Comment 4•7 years ago
|
||
I did some refactoring while working on this that isn't related to fixing the bug. I'll post those patches first.
Assignee | ||
Comment 5•7 years ago
|
||
ScriptLoadRequests have the flags mIsDefer and mIsAsync. These don't exactly reflect the script attributes however, but which list the request is currently in. This patch renames them to make this more obvious.
Attachment #8939887 -
Flags: review?(bugs)
Assignee | ||
Comment 6•7 years ago
|
||
Patch to factor out a method to add an async request to the appropriate list. I moved MaybeMoveToLoadedList to just after this as that deals with moving async requests between lists.
Attachment #8939888 -
Flags: review?(bugs)
Assignee | ||
Comment 7•7 years ago
|
||
Patch to add an enum for the script processing mode, one of Blocking, Deferred or Async. This is stored on the load request and means we can get rid of the separate flags for this used for preloading. We set the mode to Deferred for module scripts by default and propagate the mode to module import requests.
Attachment #8939891 -
Flags: review?(bugs)
Assignee | ||
Comment 8•7 years ago
|
||
And this is the patch to fix the problem. This adds an mIsModule flag to HTMLScriptElement which is frozen along with other attributes related to script execution in HTMLScriptElement::FreezeUriAsyncDefer, now renamed FreezeExecutionAttrs. We can then check this to see whether the async attribute is allowed on an inline script. This required moving ModuleScriptsEnabled out of ScriptLoader and adding a parameter for the owning document.
Attachment #8939918 -
Flags: review?(bugs)
Assignee | ||
Comment 9•7 years ago
|
||
With these patches I see the fast scripts loaded before the slow ones when following the link in comment 2.
Updated•7 years ago
|
Attachment #8939887 -
Flags: review?(bugs) → review+
Updated•7 years ago
|
Attachment #8939888 -
Flags: review?(bugs) → review+
Comment 10•7 years ago
|
||
Comment on attachment 8939891 [details] [diff] [review] bug1361369-3-script-mode >+ enum class ScriptMode : uint8_t { nit, { should be on its own line >+ Blocking, >+ Deferred, >+ Async I'd prefer e-prefix. eBlocking etc. That is what the coding style also says. >+ }; >+ >+ void SetScriptMode(bool aDeferAttr, bool aAsyncAttr); >+ >+ bool IsBlockingScript() const { { should be on its own line after method definition. Same also elsewhere >+++ b/dom/script/ScriptLoader.cpp >@@ -1090,44 +1090,42 @@ ScriptLoader::StartLoad(ScriptLoadReques > // The following tell the cache to look for an alternative data type which > // does not exist, such that we can later save the bytecode with a > // different alternative data type. > LOG(("ScriptLoadRequest (%p): Request saving bytecode later", aRequest)); > cic->PreferAlternativeDataType(kNullMimeType); > } > } > >- nsIScriptElement* script = aRequest->mElement; >- bool async = script ? script->GetScriptAsync() : aRequest->mPreloadAsAsync; >- bool defer = script ? script->GetScriptDeferred() : aRequest->mPreloadAsDefer; >- >- LOG(("ScriptLoadRequest (%p): async=%d defer=%d tracking=%d", >- aRequest, async, defer, aRequest->IsTracking())); >+ LOG(("ScriptLoadRequest (%p): mode=%d tracking=%d", >+ aRequest, unsigned(aRequest->mScriptMode), aRequest->IsTracking())); ensure that compiler doesn't warn about this. > > nsCOMPtr<nsIClassOfService> cos(do_QueryInterface(channel)); > if (cos) { >- if (aRequest->mScriptFromHead && !async && !defer) { >+ if (aRequest->mScriptFromHead && aRequest->IsBlockingScript()) >+ { { should be on the previous line
Attachment #8939891 -
Flags: review?(bugs) → review+
Comment 11•7 years ago
|
||
Comment on attachment 8939918 [details] [diff] [review] bug1361369-4-async-inline-module-scripts >+bool >+mozilla::dom::ModuleScriptsEnabled(nsIDocument* aOwnerDoc) >+{ >+ static bool sEnabledForContent = false; >+ static bool sCachedPref = false; >+ if (!sCachedPref) { >+ sCachedPref = true; >+ Preferences::AddBoolVarCache(&sEnabledForContent, "dom.moduleScripts.enabled", false); >+ } >+ >+ return nsContentUtils::IsChromeDoc(aOwnerDoc) || sEnabledForContent; >+} This should be just a method on nsIDocument. Then there was no need for the param. Implementation would go to nsDocument.cpp >+++ b/dom/script/ScriptElement.h >@@ -47,12 +47,15 @@ protected: > /** > * Check if this element contains any script, linked or inline > */ > virtual bool HasScriptContent() = 0; > > virtual bool MaybeProcessScript() override; > }; > >+// Return whether module scripts are enabled for a given document. >+extern bool ModuleScriptsEnabled(nsIDocument* aOwnerDoc); So, move this to nsIDocument class and drop extern and the param We need some tests here. I guess some mochitest with .sjs on server side to slow down processing or something.
Attachment #8939918 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 12•7 years ago
|
||
Updated patch with review comments addressed.
Attachment #8939891 -
Attachment is obsolete: true
Attachment #8940701 -
Flags: review+
Assignee | ||
Comment 13•7 years ago
|
||
There were a bunch of other places in ScriptLoadRequest.h that had the same style issues. This patch fixes those.
Attachment #8940702 -
Flags: review?(bugs)
Assignee | ||
Comment 14•7 years ago
|
||
Updated patch with review comments addressed.
Attachment #8940703 -
Flags: review+
Assignee | ||
Comment 15•7 years ago
|
||
Patch to add a test based on the test case in this bug.
Attachment #8939918 -
Attachment is obsolete: true
Attachment #8940704 -
Flags: review?(bugs)
Updated•7 years ago
|
Attachment #8940702 -
Flags: review?(bugs) → review+
Comment 16•7 years ago
|
||
Comment on attachment 8940704 [details] [diff] [review] bug1361369-5-tests oh, interesting, I didn't know wpt can do that. + <script type="module" async> + import "./resources/fast-module.js?unique=2"; + loaded.push("fast"); + </script> is indented too much. But shouldn't you test also the case when page has <script type="module"> elements without async. Additional patch on top of this perhaps?
Attachment #8940704 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 17•7 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #16) > But shouldn't you test also the case when page has <script type="module"> elements without async That's covered by the execorder.html test in that directory, as far as I can work out.
Comment 18•7 years ago
|
||
Pushed by jcoppeard@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/43b6dc38e985 Rename some script load request flags to be more descriptive r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/b8bae6e72f42 Factor out method to queue an async request r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/465f1a161695 Add a script processing mode enum r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/697b4f431bf0 Fix coding style in ScriptLoadRequest.h r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/d4a7a8196fc3 Allow async attribute on inline module scripts r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/db30c73b1542 Add a test for execution order of inline async module scripts r=smaug
Comment 19•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/43b6dc38e985 https://hg.mozilla.org/mozilla-central/rev/b8bae6e72f42 https://hg.mozilla.org/mozilla-central/rev/465f1a161695 https://hg.mozilla.org/mozilla-central/rev/697b4f431bf0 https://hg.mozilla.org/mozilla-central/rev/d4a7a8196fc3 https://hg.mozilla.org/mozilla-central/rev/db30c73b1542
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in
before you can comment on or make changes to this bug.
Description
•