Support async on inline module scripts

RESOLVED FIXED in Firefox 59



2 years ago
2 years ago


(Reporter: jaffathecake, Assigned: jonco)



Dependency tree / graph

Firefox Tracking Flags

(firefox59 fixed)



(6 attachments, 2 obsolete attachments)

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:

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
Keywords: triage-deferred
Priority: -- → P3
Do you still have the example to reproduce this?  That link doesn't work any more.
Flags: needinfo?(jaffathecake)
Sorry. Updated link:
Flags: needinfo?(jaffathecake)
I can reproduce this.  It seems the async attribute is not propagated to module imports.
Assignee: nobody → jcoppeard
Ever confirmed: true
I did some refactoring while working on this that isn't related to fixing the bug.  I'll post those patches first.
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)
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)
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)
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)
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]

>+  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]

>+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+
Updated patch with review comments addressed.
Attachment #8939891 - Attachment is obsolete: true
Attachment #8940701 - Flags: review+
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)
Updated patch with review comments addressed.
Attachment #8940703 - Flags: review+
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]

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+
(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.
Pushed by
Rename some script load request flags to be more descriptive r=smaug
Factor out method to queue an async request r=smaug
Add a script processing mode enum r=smaug
Fix coding style in ScriptLoadRequest.h r=smaug
Allow async attribute on inline module scripts r=smaug
Add a test for execution order of inline async module scripts r=smaug
Depends on: 1429783
You need to log in before you can comment on or make changes to this bug.