Support async on inline module scripts

RESOLVED FIXED in Firefox 59

Status

()

P3
enhancement
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: jaffathecake, Assigned: jonco)

Tracking

({triage-deferred})

Trunk
mozilla59
triage-deferred
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox59 fixed)

Details

Attachments

(6 attachments, 2 obsolete attachments)

(Reporter)

Description

2 years ago
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.
Component: Untriaged → JavaScript Engine
Product: Firefox → Core
Severity: normal → enhancement
Blocks: 1240072
Keywords: triage-deferred
Priority: -- → P3
(Assignee)

Comment 1

a year ago
Do you still have the example to reproduce this?  That link doesn't work any more.
Flags: needinfo?(jaffathecake)
(Reporter)

Comment 2

a year ago
Sorry. Updated link: https://module-script-tests-sreyfhwvpq.now.sh/async
Flags: needinfo?(jaffathecake)
(Assignee)

Comment 3

a year ago
I can reproduce this.  It seems the async attribute is not propagated to module imports.
Assignee: nobody → jcoppeard
(Assignee)

Updated

a year ago
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(Assignee)

Comment 4

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

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

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

a year ago
Posted patch bug1361369-3-script-mode (obsolete) — Splinter Review
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

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

a year ago
With these patches I see the fast scripts loaded before the slow ones when following the link in comment 2.
Attachment #8939887 - Flags: review?(bugs) → review+
Attachment #8939888 - Flags: review?(bugs) → review+
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 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

a year ago
Updated patch with review comments addressed.
Attachment #8939891 - Attachment is obsolete: true
Attachment #8940701 - Flags: review+
(Assignee)

Comment 13

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

a year ago
Updated patch with review comments addressed.
Attachment #8940703 - Flags: review+
(Assignee)

Comment 15

a year 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)
Attachment #8940702 - Flags: review?(bugs) → review+
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

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

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

Updated

a year ago
Depends on: 1429783
You need to log in before you can comment on or make changes to this bug.