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/

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

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

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0d9ae32d14c0f5ee507ae9d3b3d4b135bcba6d0c need to find a reviewer.

(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

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?

(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.

(sorry to change it potentially under your hands) https://treeherder.mozilla.org/#/jobs?repo=try&revision=cbb0c3df20d265dc47c9cfacc43ce5ebcf460fb5

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.

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!

- nsHtml5SpeculativeLoad members moved and inited in release (good catch, I missed that #define DEBUG completely!)

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!

Thanks Ben for reviews! One more push since I've seen some failure with the previous version of the test.

(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

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

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

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.

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.

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.

(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.

The try push looks good, let's get it in again.

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

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.

(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?

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

(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?

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.

(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.

Just discovered this has not landed on mozilla-central correctly at all!!!

Confirmed this is on 56 Beta now.

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...

I checked the try run: no eslint errors, no unexpected android failures.

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

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.

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.