Closed Bug 1053321 Opened 6 years ago Closed 3 years ago

Defer loaded JavaScript blocks parallel download of following resources

Categories

(Core :: DOM: Core & HTML, defect, P1)

31 Branch
x86
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox55 --- wontfix
firefox56 + fixed
firefox57 --- fixed

People

(Reporter: services, Assigned: mayhemer)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 6 obsolete files)

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/
Summary: Defer loaded JavaScript blocks parallel download of following ressources → Defer loaded JavaScript blocks parallel download of following resources
Product: Firefox → Core
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.
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
Blocks: CDP
Priority: -- → P3
Depends on: tailing
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]
And btw, the same applies to async scripts.
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
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.
(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
Attachment #8891669 - Flags: review?(bkelly)
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)
(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)
Attached 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+
Blocks: 1386194
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!
Attached 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+
Attached 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+
(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
Attached 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+
Keywords: checkin-needed
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
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.
https://hg.mozilla.org/mozilla-central/rev/548ed79f2337
Status: ASSIGNED → RESOLVED
Closed: 3 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 → ---
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
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)
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.
(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.
Blocks: 1386644
The try push looks good, let's get it in again.
Keywords: checkin-needed
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.
(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.
> Should we just make it an enum?

Make which an enum?
https://hg.mozilla.org/mozilla-central/rev/17e524a6c990
Status: ASSIGNED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
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.
(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-
Just discovered this has not landed on mozilla-central correctly at all!!!
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Confirmed this is on 56 Beta now.
Flags: qe-verify-
Flags: in-testsuite+
Target Milestone: mozilla57 → ---
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+
Attachment #8892859 - Attachment is obsolete: true
I checked the try run: no eslint errors, no unexpected android failures.
Keywords: checkin-needed
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)
https://hg.mozilla.org/mozilla-central/rev/b3c222670334
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.