Defer loaded JavaScript blocks parallel download of following resources

RESOLVED FIXED in Firefox 56

Status

()

defect
P1
normal
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: services, Assigned: mayhemer)

Tracking

(Blocks 1 bug)

31 Branch
mozilla57
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 wontfix, firefox56+ fixed, firefox57 fixed)

Details

Attachments

(1 attachment, 6 obsolete attachments)

Reporter

Description

5 years ago
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:31.0) Gecko/20100101 Firefox/31.0 (Beta/Release)
Build ID: 20140716183446

Steps to reproduce:

Created the following test page:

HEAD: External JavaScript using HTML tag with defer
BODY: 4 pictures

Testpage: stevesouders.com/cuzillion/?c0=hj1hfft2_0_f&c1=b...f&c3=bi1hfff2_0_f&c4=bi1hfff2_0_f&t=1407883291

Had a look at the waterfall graph with WebPagetest.org and Firebug Network monitor.


Actual results:

The first HTTP Request for the external JavaScript file (with defer) blocks in Firefox the parallel download of the 4 pictures in the body.

Test result: http://www.webpagetest.org/result/140812_NE_17QM/1/details/


Expected results:

Loading the external JavaScript with defer should not block downloading the pictures in parallel. Like it is in

Chrome: http://www.webpagetest.org/result/140812_3S_17QK/1/details/
Safari: http://www.webpagetest.org/result/140812_FE_17QN/1/details/
IE9: http://www.webpagetest.org/result/140812_03_17QP/1/details/
IE11: http://www.webpagetest.org/result/140812_SX_17QR/1/details/
Reporter

Updated

5 years ago
Summary: Defer loaded JavaScript blocks parallel download of following ressources → Defer loaded JavaScript blocks parallel download of following resources

Updated

5 years ago
Component: Untriaged → Untriaged
Product: Firefox → Core

Updated

5 years ago
Component: Untriaged → Networking
can you please retest on nightly? We made a recent change there so there is reason to think it may be different.
Reporter

Comment 2

5 years ago
Still the same: http://www.webpagetest.org/result/140814_9G_ET0/1/details/

Tested with Firefox Nightly via WebPagtest.
User-Agent: Mozilla/5.0 (Windows NT 6.1; rv:34.0) Gecko/20100101 Firefox/34.0 PTST/183
Assignee: nobody → mcmanus
Whiteboard: [necko-active]
Whiteboard: [necko-active] → [necko-next]
Assignee: mcmanus → honzab.moz
Assignee

Updated

2 years ago
Blocks: CDP
Priority: -- → P3
Assignee

Updated

2 years ago
Depends on: tailing
Assignee

Comment 3

2 years ago
This definitely doesn't depend on the Tail feature.  What we do here is that we treat the head referenced JS (regardless it's deferred) as blocking, hence we don't allow any resource referenced from body to load.  

If there were actual bandwidth utilization caused by parallel loading images, we would delay DOMContentLoaded, however, we would not block first paint.  Hence it doesn't make sense to mark js deferred loads as blocking.

Firefox:
http://www.webpagetest.org/result/170729_7T_VY2/1/details/#waterfall_view_step1

Chrome:
http://www.webpagetest.org/result/170729_CG_VXE/1/details/#waterfall_view_step1
Status: UNCONFIRMED → ASSIGNED
No longer depends on: tailing
Ever confirmed: true
Whiteboard: [necko-next] → [necko-active]
Assignee

Comment 4

2 years ago
And btw, the same applies to async scripts.
Assignee

Comment 5

2 years ago
The problem is specifically at [1].  The speculative load is put to the queue w/o a knowledge whether the script is async or deferred.  Note that at [2] the mElement member is null, hence we can't determine.

[1] https://dxr.mozilla.org/mozilla-central/rev/36f95aeb4c77f7cf3b3366583008cd6e4b6b1dba/parser/html/nsHtml5TreeBuilderCppSupplement.h#177
[2] https://dxr.mozilla.org/mozilla-central/rev/36f95aeb4c77f7cf3b3366583008cd6e4b6b1dba/dom/script/ScriptLoader.cpp#974
Assignee

Updated

2 years ago
Component: Networking → DOM: Core & HTML
Priority: P3 → P1
Whiteboard: [necko-active]
(In reply to Honza Bambas (:mayhemer) from comment #6)
> need to find a reviewer.

I can take this for review if you don't find anyone better.  A link to the relevant spec (if any) would be helpful.
Assignee

Comment 8

2 years ago
(In reply to Ben Kelly [:bkelly] from comment #7)
> (In reply to Honza Bambas (:mayhemer) from comment #6)
> > need to find a reviewer.
> 
> I can take this for review if you don't find anyone better.  A link to the
> relevant spec (if any) would be helpful.

Thank you!

Let you know that we already have the code for the right load classification for a very long time, but it has been broken because the necessary pointer on the request object was null (still is).  So it's more about checking the desired functionality of our code than checking we conform the spec.

But just in case, the documentation: https://developer.mozilla.org/en/docs/Web/HTML/Element/script, and w3c sped at https://www.w3.org/TR/html5/scripting-1.html#attr-script-defer
Assignee

Updated

2 years ago
Attachment #8891669 - Flags: review?(bkelly)

Comment 9

2 years ago
Could we add a test for not accidentally breaking this again, perhaps by using sjs files or similar to verify the order in which we make requests?
Flags: needinfo?(honzab.moz)
Assignee

Comment 10

2 years ago
(In reply to :Gijs from comment #9)
> Could we add a test for not accidentally breaking this again, perhaps by
> using sjs files or similar to verify the order in which we make requests?

I can think of something, yes.
Flags: needinfo?(honzab.moz)
Assignee

Comment 11

2 years ago
Posted patch v1 + test (obsolete) — Splinter Review
(sorry to change it potentially under your hands)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cbb0c3df20d265dc47c9cfacc43ce5ebcf460fb5
Attachment #8891669 - Attachment is obsolete: true
Attachment #8891669 - Flags: review?(bkelly)
Attachment #8892035 - Flags: review?(bkelly)
Comment on attachment 8892035 [details] [diff] [review]
v1 + test

Review of attachment 8892035 [details] [diff] [review]:
-----------------------------------------------------------------

Overall the changes look good, but I think we should write a WPT test here.

::: dom/tests/mochitest/script/test_bug1053321.html
@@ +28,5 @@
> +    document.addEventListener("DOMContentLoaded", function() {
> +      ok(window.script_executed_defer, "Deferred script executed before DOMContentLoaded");
> +    });
> +    window.addEventListener("load", function() {
> +      ok(window.script_executed_async, "Async script executed before onload");

Don't you need SimpleTest.waitForFinish() and SimpleTest.finish() here?  Or does mochitest without those wait for the load event?  I didn't think it did.

::: dom/tests/moz.build
@@ +97,5 @@
>  with Files("mochitest/pointerlock/**"):
>      BUG_COMPONENT = ("Core", "DOM")
>  
> +with Files("mochitest/script/**"):
> +    BUG_COMPONENT = ("Core", "DOM: Core & HTML")

I'm sorry, but can you write this as a WPT test?  I believe you can achieve something similar using a .py instead of the .sjs.  Since this is web-visible it would be helpful to have this in the cross-browser tests.

You can put the WPT test here:

http://searchfox.org/mozilla-central/source/testing/web-platform/tests/preload

An example of the kind of .py you can write are:

http://searchfox.org/mozilla-central/source/testing/web-platform/tests/fetch/api/resources/preflight.py#42
http://searchfox.org/mozilla-central/source/testing/web-platform/tests/fetch/api/resources/clean-stash.py#3

I linked lines where I think its using server side state like you are in the .sjs.

If you need to keep it as a mochitest, thoguh, please put it in `dom/script/test` instead of under `dom/test/mochitest`.  I think we are moving towards that approach for new tests.

::: parser/html/nsHtml5SpeculativeLoad.cpp
@@ +8,5 @@
>  nsHtml5SpeculativeLoad::nsHtml5SpeculativeLoad()
>  #ifdef DEBUG
>   : mOpCode(eSpeculativeLoadUninitialized)
> + , mIsAsync(false)
> + , mIsDefer(false)

Initializing data members in a #ifdef DEBUG seems unnecessarily risky to me.  Can we just initialize this stuff always?  Is it really so hot that we must take this risk?

::: parser/html/nsHtml5SpeculativeLoad.h
@@ +277,5 @@
> +    /**
> +     * Whether the refering element has async and/or defer attributes
> +     */
> +    bool mIsAsync;
> +    bool mIsDefer;

It would be nice to group these with mOpCode to get better packing.
Attachment #8892035 - Flags: review?(bkelly) → feedback+
Assignee

Updated

2 years ago
Blocks: 1386194
Assignee

Comment 13

2 years ago
Ben, I filed bug 1386194 to write the test, since right now I won't have more cycles to work it.  I've already spent a lot of time on the mochitest and I believe (with slight modifications) it's sufficient to check this is not broken.  Note that this is about network prioritization and not about web compatibility.

I'll update the code changes and rerequest w/o a test included for now.  Thanks!
Assignee

Comment 14

2 years ago
Posted patch v1.1 (no test) (obsolete) — Splinter Review
- nsHtml5SpeculativeLoad members moved and inited in release (good catch, I missed that #define DEBUG completely!)
Attachment #8892035 - Attachment is obsolete: true
Attachment #8892403 - Flags: review?(bkelly)
Comment on attachment 8892403 [details] [diff] [review]
v1.1 (no test)

Review of attachment 8892403 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

Feel free to land the mochitest as well in dom/script/test.  Thanks!
Attachment #8892403 - Flags: review?(bkelly) → review+
Assignee

Comment 16

2 years ago
Posted patch v1.2 + mochitest (obsolete) — Splinter Review
Thanks Ben for reviews!

One more push since I've seen some failure with the previous version of the test.
Attachment #8892403 - Attachment is obsolete: true
Attachment #8892557 - Flags: review+
Assignee

Comment 17

2 years ago
(In reply to Honza Bambas (:mayhemer) from comment #16)
> Created attachment 8892557 [details] [diff] [review]
> One more push since I've seen some failure with the previous version of the
> test.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2804ff11e028e243942a4ad5faabff62ecb32d4d
Assignee

Comment 18

2 years ago
Posted patch 1.2.1 + mochitest (obsolete) — Splinter Review
last full successful push module android mochitest failures:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0d9ae32d14c0f5ee507ae9d3b3d4b135bcba6d0c

affected mochitest only re-push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=aaf614ee62e050afc02a29dc0512b73dfe285c92
Attachment #8892557 - Attachment is obsolete: true
Attachment #8892597 - Flags: review+
Assignee

Updated

2 years ago
Keywords: checkin-needed

Comment 19

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/20344f0dbff9
Correctly pass info about 'defer' and 'async' attributes to HTML5 parser invoked script preload. r=bkelly
Keywords: checkin-needed

Comment 20

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/732e73a9c657
Backed out changeset 20344f0dbff9 for ESLint failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/548ed79f2337
Correctly pass info about 'defer' and 'async' attributes to HTML5 parser invoked script preload. r=bkelly
The attached patch landed in comment 19 had ESLint failures. It was backed out and the version from the last Try push was re-landed instead.

Comment 22

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/548ed79f2337
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Oh, fun, we merged around this bustage?

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/70bd3854b6d0, and then I guess I get to back it out again on mozilla-central, for the strange and terrible Android failures of https://treeherder.mozilla.org/logviewer.html#?job_id=120100424&repo=mozilla-inbound and https://treeherder.mozilla.org/logviewer.html#?job_id=120094787&repo=mozilla-inbound

Pay no attention to the suggested intermittent bug, or that false-positive noise, or the fact that they were starred all day and merged around: the part to pay attention to is the way it reports "Failed: 1" despite apparently eating every shred of a sign of which one test failed.
Status: RESOLVED → REOPENED
Flags: needinfo?(honzab.moz)
Resolution: FIXED → ---
Target Milestone: mozilla56 → ---

Comment 24

2 years ago
Backout by philringnalda@gmail.com:
https://hg.mozilla.org/mozilla-central/rev/320642944e42
Backed out changeset 548ed79f2337 for strange and terribly-reported failures in Android opt mochitest-15 and debug mochitest-36
Assignee

Comment 25

2 years ago
Interesting:


[task 2017-08-01T21:19:48.039792Z] 21:19:48     INFO -  279 INFO TEST-START | dom/tests/mochitest/script/test_bug1053321.html
[task 2017-08-01T21:19:48.120474Z] 21:19:48     INFO -  *** error running SJS at /home/worker/workspace/build/tests/mochitest/tests/dom/tests/mochitest/script/file_blocked_script.sjs: 2147746065 on line NaN
[task 2017-08-01T21:19:48.140620Z] 21:19:48     INFO -  *** error running SJS at /home/worker/workspace/build/tests/mochitest/tests/dom/tests/mochitest/script/file_blocked_script.sjs: 2147746065 on line NaN
[task 2017-08-01T21:20:00.739898Z] 21:20:00     INFO -  280 INFO TEST-OK | dom/tests/mochitest/script/test_bug1053321.html | took 3399ms
[task 2017-08-01T21:20:00.740024Z] 21:20:00     INFO -  281 ERROR /tests/dom/tests/mochitest/script/test_bug1053321.html logged result after SimpleTest.finish(): Async script executed before onload

2147746065 == NS_ERROR_NOT_AVAILABLE.



OK, since I'd like this patch be in 56, I'll land w/o the test for now.  We want a WPT test anyway (this is a mochitest causing troubles), separate bug files for that.
Flags: needinfo?(honzab.moz)
Assignee

Updated

2 years ago
Status: REOPENED → ASSIGNED
(In reply to Honza Bambas (:mayhemer) from comment #25)
> OK, since I'd like this patch be in 56, I'll land w/o the test for now.  We
> want a WPT test anyway (this is a mochitest causing troubles), separate bug
> files for that.

I think it would be better to disable on android rather than land with zero test coverage.
Assignee

Comment 28

2 years ago
(In reply to Ben Kelly [:bkelly] from comment #27)
> (In reply to Honza Bambas (:mayhemer) from comment #25)
> > OK, since I'd like this patch be in 56, I'll land w/o the test for now.  We
> > want a WPT test anyway (this is a mochitest causing troubles), separate bug
> > files for that.
> 
> I think it would be better to disable on android rather than land with zero
> test coverage.

I'll do that separately.
Assignee

Updated

2 years ago
Blocks: 1386644
Assignee

Comment 29

2 years ago
The try push looks good, let's get it in again.
Keywords: checkin-needed

Comment 30

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/17e524a6c990
Correctly pass info about 'defer' and 'async' attributes to HTML5 parser invoked script preload. r=bkelly
Keywords: checkin-needed
I discussed this with bkelly and we decided to re-land the original patch from comment 22 with the Android-only skip-if instead rather than have no test coverage at all.
Hrm.  Henri, given that we now have these new booleans from this bug in the operation object, do you still think that in bug 1382020 we want to add new opcodes?  We now have a hole to just put a boolean into....

And creating a set of opcodes for three orthogonal booleans (times the 4 opcodes we have now) seems pretty excessive.
Flags: needinfo?(hsivonen)
(In reply to Boris Zbarsky [:bz] from comment #32)
> Hrm.  Henri, given that we now have these new booleans from this bug in the
> operation object, do you still think that in bug 1382020 we want to add new
> opcodes?  We now have a hole to just put a boolean into....
> 
> And creating a set of opcodes for three orthogonal booleans (times the 4
> opcodes we have now) seems pretty excessive.

Should we just make it an enum?  I considered asking for that in the review, but this seemed kind of urgent to fix.
Assignee

Comment 34

2 years ago
(In reply to Ben Kelly [:bkelly] from comment #33)
> (In reply to Boris Zbarsky [:bz] from comment #32)
> > Hrm.  Henri, given that we now have these new booleans from this bug in the
> > operation object, do you still think that in bug 1382020 we want to add new
> > opcodes?  We now have a hole to just put a boolean into....
> > 
> > And creating a set of opcodes for three orthogonal booleans (times the 4
> > opcodes we have now) seems pretty excessive.
> 
> Should we just make it an enum?  I considered asking for that in the review,
> but this seemed kind of urgent to fix.

Thank you.  Hopefully this sticks.
Comment hidden (obsolete)
> Should we just make it an enum?

Make which an enum?

Comment 37

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/17e524a6c990
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Assignee

Comment 38

2 years ago
Comment on attachment 8892859 [details] [diff] [review]
v1.2.1 (no test)

This would be nice to get to 56 for beta testing, but it's not critical.  I missed the merge date just because of a badly written test for this feature.  If not found feasible for 56 uplift, leave it ride.

Approval Request Comment
[Feature/Bug causing the regression]: not sure when this has regressed exactly
[User impact if declined]: perceived slowdown of page subresources load (namely effects fonts and images)
[Is this code covered by automated tests?]: it is!
[Has the fix been verified in Nightly?]: yes, locally and via the test
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: this is only a prioritization change which other browsers (Chrome/Edge) already do for a long time
[String changes made/needed]: none
Attachment #8892859 - Flags: approval-mozilla-beta?
(In reply to Boris Zbarsky [:bz] from comment #32)
> Hrm.  Henri, given that we now have these new booleans from this bug in the
> operation object, do you still think that in bug 1382020 we want to add new
> opcodes?  We now have a hole to just put a boolean into....
> 
> And creating a set of opcodes for three orthogonal booleans (times the 4
> opcodes we have now) seems pretty excessive.

Yeah, if we have this many booleans, we should have real booleans (as in the patch) and also turn the "in head" bit of info into a boolean instead of an enum case. Is there already a follow-up for that?
Flags: needinfo?(hsivonen)
Let's see how this does in nightly and then plan to uplift it for beta 2 or 3. 
Feels like uplifting this right now might risk delaying beta 1.
Assignee

Comment 41

2 years ago
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #40)
> Let's see how this does in nightly and then plan to uplift it for beta 2 or
> 3. 
> Feels like uplifting this right now might risk delaying beta 1.

Let you know this is not critical at all.  Just would be good to have beta testing prior 57.  Thanks.
Comment on attachment 8892859 [details] [diff] [review]
v1.2.1 (no test)

Let's try this for beta 2.
Attachment #8892859 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to Honza Bambas (:mayhemer) from comment #38)
> [Is this code covered by automated tests?]: it is!
> [Has the fix been verified in Nightly?]: yes, locally and via the test
> [Needs manual test from QE? If yes, steps to reproduce]: no

Setting qe-verify- based on Honza's assessment on manual testing needs and the fact that this fix has automated coverage.
Flags: qe-verify-
Assignee

Comment 45

2 years ago
Just discovered this has not landed on mozilla-central correctly at all!!!
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee

Comment 46

2 years ago
Confirmed this is on 56 Beta now.
Flags: qe-verify-
Flags: in-testsuite+
Target Milestone: mozilla57 → ---
Assignee

Comment 47

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=54762e1ae5bbe959b89060eb41e95417578c5050

This is the closest as I can get to what has been landed on m-b and sticks there.

Please no more fiddling with the test and its location in the tree, the feature is more important to be in the tree than to have a test for it.  Thanks.

Note that https://hg.mozilla.org/mozilla-central/file/tip/dom/tests/mochitest/script/mochitest.ini was forgotten in the tree...
Attachment #8899119 - Flags: review+
Assignee

Updated

2 years ago
Attachment #8892859 - Attachment is obsolete: true
Assignee

Comment 48

2 years ago
I checked the try run: no eslint errors, no unexpected android failures.
Keywords: checkin-needed

Comment 49

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b3c222670334
Correctly pass info about 'defer' and 'async' attributes to HTML5 parser invoked script preload. r=bkelly
Keywords: checkin-needed
Greg, do you have any idea how the commit in comment 37 seems to have reverted itself? I can't make heads or tails of it. Looks like maybe some kind of weird merge issue, but even the merge diff doesn't show anything weird.
Flags: needinfo?(gps)
Depends on: 1392382
It smells like a Mercurial bug. Filed https://bz.mercurial-scm.org/show_bug.cgi?id=5665.

Probably no need to panic. This was a somewhat wonky merge with the backout and a re-land being in the p2 ancestry and there being another backout before that in the p1 ancestry.
Flags: needinfo?(gps)

Comment 52

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b3c222670334
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.