Closed Bug 1361369 Opened 7 years ago Closed 6 years ago

Support async on inline module scripts

Categories

(Core :: JavaScript Engine, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: jaffathecake, Assigned: jonco)

References

Details

(Keywords: triage-deferred)

Attachments

(6 files, 2 obsolete files)

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: modules-0
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: https://module-script-tests-sreyfhwvpq.now.sh/async
Flags: needinfo?(jaffathecake)
I can reproduce this.  It seems the async attribute is not propagated to module imports.
Assignee: nobody → jcoppeard
Status: UNCONFIRMED → ASSIGNED
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)
Attached 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]
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+
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]
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+
(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 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
Depends on: 1429783
You need to log in before you can comment on or make changes to this bug.