591981 - Don't enforce execute-in-insertion-order for DOM inserted <script>s

Status: RESOLVED FIXED

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

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

No other browsers do this, and the HTML5 spec says not to do this.

Also, which makes this more urgent, is that the way we now follow the HTML5 spec for document.write makes it more critical that we align with other browsers on script execution as to avoid websites breaking.

Comment 1

[Henri Sivonen (:hsivonen)]

Attachment: Untested WIP

Comment 2

[Henri Sivonen (:hsivonen)]

Test failures:
4682 ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/test_bug28293.html | scripts executed in the wrong order - got "AacBCDEFGeHIJfb1dM2g3i", expected "AacBCDEFGeHIJfbd1M2g34hi"
4683 ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/test_bug28293.html | Dynamic script executed too late
4686 ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/test_bug28293.xhtml | [SimpleTest/SimpleTest.js, window.onerror] An error occurred - Error loading script at http://mochi.test:8888/tests/content/base/test/file_bug28293.sjs?res+=%27h%27;:1
36423 ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/content/test/test_bug300691-2.html | Setting src should execute script - got false, expected true
36424 ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/content/test/test_bug300691-2.html | Setting src attribute should execute script - got false, expected true
36425 ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/content/test/test_bug300691-2.html | src attribute takes precedence over inline content - got false, expected true
36427 ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/content/test/test_bug300691-2.html | src attribute load should have started before the attribute got removed - got false, expected true
6516 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/bugs/test_bug260264_nested.html | Test timed out.
6574 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/bugs/test_bug346659.html | [SimpleTest/SimpleTest.js, window.onerror] An error occurred - win is null at http://mochi.test:8888/tests/dom/tests/mochitest/bugs/test_bug346659.html:163
7904 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/bugs/test_bug414291.html | window didn't open as modal. - got null, expected 3
7957 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/bugs/test_bug437361.html | Problem with modal dialog returnValue. - got null, expected 1
7978 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/bugs/test_bug458091.html | [SimpleTest/SimpleTest.js, window.onerror] An error occurred - gTestWin is null at http://mochi.test:8888/tests/dom/tests/mochitest/bugs/test_bug458091.html:75
8014 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/bugs/test_bug49312.html | Test timed out.
8063 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/bugs/test_bug562433.html | [SimpleTest/SimpleTest.js, window.onerror] An error occurred - w is null at http://mochi.test:8888/tests/dom/tests/mochitest/bugs/test_bug562433.html:28

Comment 3

[Henri Sivonen (:hsivonen)]

Follow-up: bug 592366.

Comment 4

[Henri Sivonen (:hsivonen)]

Another follow-up: bug 592656.

Comment 5

[Henri Sivonen (:hsivonen)]

Attachment: Fix

Comment 6

[Henri Sivonen (:hsivonen)]

Comment on attachment 471101 [details] [diff] [review]
Fix

In addition to the must-change stuff, I renamed some bits to make it clearer what the fields or methods really mean.

Comment 7

[Henri Sivonen (:hsivonen)]

This patch causes the following reftest failures:
REFTEST TEST-UNEXPECTED-FAIL | http://localhost:4444/1283778336789/7/reftest-sanity/filter-1.xhtml | timed out waiting for onload to fire
REFTEST TEST-UNEXPECTED-FAIL | http://localhost:4444/1283778336789/8/reftest-sanity/filter-2.xhtml | timed out waiting for onload to fire
REFTEST TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/tryserver_fedora64_test-reftest/build/reftest/tests/layout/reftests/css-valuesandunits/unit-rem.svg | timed out waiting for onload to fire
REFTEST TEST-UNEXPECTED-FAIL | http://localhost:4444/1283778337494/130/ogg-video/aspect-ratio-1a.xhtml | timed out waiting for onload to fire
REFTEST TEST-UNEXPECTED-FAIL | http://localhost:4444/1283778337495/131/ogg-video/aspect-ratio-1b.xhtml | timed out waiting for onload to fire
REFTEST TEST-UNEXPECTED-FAIL | http://localhost:4444/1283778337496/134/ogg-video/aspect-ratio-3a.xhtml | timed out waiting for onload to fire
REFTEST TEST-UNEXPECTED-FAIL | http://localhost:4444/1283778337496/135/ogg-video/aspect-ratio-3b.xhtml | timed out waiting for onload to fire
REFTEST TEST-UNEXPECTED-FAIL | http://localhost:4444/1283778337497/136/ogg-video/basic-1.xhtml | timed out waiting for onload to fire
REFTEST TEST-UNEXPECTED-FAIL | http://localhost:4444/1283778337497/137/ogg-video/canvas-1a.xhtml | timed out waiting for onload to fire
REFTEST TEST-UNEXPECTED-FAIL | http://localhost:4444/1283778337497/138/ogg-video/canvas-1b.xhtml | timed out waiting for onload to fire
REFTEST TEST-UNEXPECTED-FAIL | http://localhost:4444/1283778337498/139/ogg-video/object-aspect-ratio-1a.xhtml | timed out waiting for onload to fire
REFTEST TEST-UNEXPECTED-FAIL | http://localhost:4444/1283778337498/140/ogg-video/object-aspect-ratio-1b.xhtml | timed out waiting for onload to fire
REFTEST TEST-UNEXPECTED-FAIL | http://localhost:4444/1283778337498/141/ogg-video/offset-1.xhtml | timed out waiting for onload to fire
REFTEST TEST-UNEXPECTED-FAIL | http://localhost:4444/1283778337499/142/ogg-video/object-aspect-ratio-2a.xhtml | timed out waiting for onload to fire
REFTEST TEST-UNEXPECTED-FAIL | http://localhost:4444/1283778337499/143/ogg-video/object-aspect-ratio-2b.xhtml | timed out waiting for onload to fire
REFTEST TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/tryserver_fedora64_test-reftest/build/reftest/tests/layout/reftests/svg/svg-integration/clipPath-html-01-extref.xhtml | timed out waiting for onload to fire
REFTEST TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/tryserver_fedora64_test-reftest/build/reftest/tests/layout/reftests/svg/svg-integration/clipPath-html-02-extref.xhtml | timed out waiting for onload to fire
REFTEST TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/tryserver_fedora64_test-reftest/build/reftest/tests/layout/reftests/svg/svg-integration/clipPath-html-03-extref.xhtml | timed out waiting for onload to fire
REFTEST TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/tryserver_fedora64_test-reftest/build/reftest/tests/layout/reftests/svg/svg-integration/clipPath-html-04-extref.xhtml | timed out waiting for onload to fire
REFTEST TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/tryserver_fedora64_test-reftest/build/reftest/tests/layout/reftests/svg/svg-integration/clipPath-html-05-extref.xhtml | timed out waiting for onload to fire
REFTEST TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/tryserver_fedora64_test-reftest/build/reftest/tests/layout/reftests/svg/svg-integration/clipPath-html-06-extref.xhtml | timed out waiting for onload to fire
REFTEST TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/tryserver_fedora64_test-reftest/build/reftest/tests/layout/reftests/svg/svg-integration/filter-html-01-extref.xhtml | timed out waiting for onload to fire
buildbot.slave.commands.TimeoutError: command timed out: 7200 seconds elapsed, killing pid 2020

These tests use <svg:use>. How could that be related to my script loading changes?

Comment 8

[Boris Zbarsky [:bzbarsky]]

Well, the most obvious situation is if you're failing to unblock onload in some cases.

I'm not sure why you renamed the execute blocker stuff, by the way; some of the execute blockers were for stylesheets, but others definitely were not....

Comment 9

[Boris Zbarsky [:bzbarsky]]

And is there a reason the new boolean member isn't PRPackedBool?

Comment 10

[Henri Sivonen (:hsivonen)]

(In reply to comment #8)
> I'm not sure why you renamed the execute blocker stuff, by the way; some of the
> execute blockers were for stylesheets, but others definitely were not....

First, I wondered if the blocker were only about the same concept as "a style sheet blocking scripts" in HTML5. I searched the codebase and (apparently incorrectly!) concluded that the blockers were only about style sheet, so I thought renaming that machinery to say it's about style sheets would be helpful.

Thanks.

Is the non-style sheet stuff in any spec?

(In reply to comment #9)
> And is there a reason the new boolean member isn't PRPackedBool?

No good reason.

Comment 11

[Boris Zbarsky [:bzbarsky]]

> Is the non-style sheet stuff in any spec?

I don't know.  That's the part about blocking kids if parents are blocked, etc.

Comment 12

[Henri Sivonen (:hsivonen)]

(In reply to comment #11)
> > Is the non-style sheet stuff in any spec?
> 
> I don't know.  That's the part about blocking kids if parents are blocked, etc.

Oh. I thought I didn't change that part, and I thought that was still about sheets blocking kids. I need to re-re-re-read what I did.

I don't see anything about <svg:use> participating in script blocking. Should I?

I re-read the patch and didn't see me adding a new boolean member. Did you mean the PRUint32 mParserCreated? That's semi-boolean. Zero means not parser-created, but there are different possible non-zero values that give detail about what sort of parser-creation happened.

Comment 13

[Boris Zbarsky [:bzbarsky]]

> I don't see anything about <svg:use> participating in script blocking.
> Should I?

No.  It doesn't, to my knowledge.

> Zero means not parser-created, but there are different possible non-zero values
> that give detail about what sort of parser-creation happened.

Ah, I see.  are there more such values than would fit in a PRUint8?

Comment 14

[Henri Sivonen (:hsivonen)]

(In reply to comment #13)
> > I don't see anything about <svg:use> participating in script blocking.
> > Should I?
> 
> No.  It doesn't, to my knowledge.

OK. Is there any non-style sheet use of the mechanisms around mBlockers other than what was introduced in bug 364692? I think I've ended up regressing bug 364692 with this patch, but my excuse is that bug 364692 seemed to change the semantics of mEnabled/SetEnabled to something other than what's documented in the source comments.

What are the intended semantics of mEnabled these days? Now it seems that it's not simply that newly-inserted scripts are rejected when mEnabled is false.

I'll try to revert all my changes that depended on the following comment being true:
/**
 * Whether the loader is enabled or not.
 * When disabled, processing of new script elements is disabled.
 * Any call to ProcessScriptElement() will fail with a return code of
 * NS_ERROR_NOT_AVAILABLE. Note that this DOES NOT disable
 * currently loading or executing scripts.
 */

Reverting those changes without understanding how they affect our spec conformance bothers me, though.

> > Zero means not parser-created, but there are different possible non-zero values
> > that give detail about what sort of parser-creation happened.
> 
> Ah, I see.  are there more such values than would fit in a PRUint8?

There are four states, so no. However, it's already PRUint32 in method arguments elsewhere in the tree, so I think changing the type would be out of scope for this bug and better deferred to a follow-up bug. (It would be truly proper for it to be an enum, but I thought making it an unsigned integer played more nicely with all the legacy code that wants to pass PR_TRUE or PR_FALSE.)

Comment 15

[Henri Sivonen (:hsivonen)]

s/mBlockers/mBlockerCount/

Comment 16

[Henri Sivonen (:hsivonen)]

Attachment: Fix that passes reftest

I'm unable to reason about the spec correctness of all aspects of this patch, because I don't know when mEnabled is false.

Comment 17

[Henri Sivonen (:hsivonen)]

Comment on attachment 473001 [details] [diff] [review]
Fix that passes reftest

OK. Reftests pass when I don't touch the changes made in bug 364692.

BTW, is this considered the sort of Web platform change that'll require super-review, too?

Comment 18

[Henri Sivonen (:hsivonen)]

Bug 594339 filed as a follow-up.

Comment 19

[Boris Zbarsky [:bzbarsky]]

> However, it's already PRUint32 in method arguments elsewhere in the tree

There's no reason the member type has to match the argument type.  I'd rather not add the extra word of bloat to all script elements if we can avoid it...

For the rest, sicking is the expert on this stuff, not me.  ccing him.

Comment 20

[Henri Sivonen (:hsivonen)]

Attachment: Fix that passes reftest and uses PRUint8 for the field

(In reply to comment #19)
> > However, it's already PRUint32 in method arguments elsewhere in the tree
> 
> There's no reason the member type has to match the argument type.  I'd rather
> not add the extra word of bloat to all script elements if we can avoid it...

OK. This new patch is the same as the previous except the member is now PRUint8.

Comment 21

[Henri Sivonen (:hsivonen)]

Attachment: Fix that passes reftest and uses PRUint8 for the field

Uploading the right version this time...

Comment 22

[Henri Sivonen (:hsivonen)]

As mentioned on IRC, I rewrote parts of the running algorith according to HTML5 instead of hacking the old code.

 * Script-inserted inline scripts always run immediately (even if there are style sheets blocking script), so there's no list for them. They run off a script-runner, because the insertion DOM modification has an update batch.
 * There can be only one parser-inserted script blocking at a time (when nodes are moved between documents, bug 592366 need fixing, though). That's why there's a single field for that instead of an array.
 * Script-inserted external scripts and parser-inserted async script work the same way, so they go into one array.
 * Decoupling defer scripts from parser-blocking scripts makes things conceptually clearer and conceptually the same as in the spec.
 * document.written scripts don't block on style sheets blocking scripts. This way, document.write() returns predictably instead of racing with style sheet loads. The details are imitated from IE for now, because Hixie didn't respond to my bug about the details in time for this beta.
 * If an external script is immediately available due to preloading, a trip through the event loop is made in order not to expose event loop differences from preloading to Web content (especially to avoid running script-inserted scripts synchronously).
 * document.written scripts might now behave differently relative to the changes made in bug 364692. I didn't understand the fix for bug 364692.
 * Otherwise, I tried to retain the fix for bug 364692, albeit in a cargo cultish way.

Comment 23

[Henri Sivonen (:hsivonen)]

About not understanding the fix for bug 364692: The specific thing that's unclear to me is the expansion of the mEnabled semantics--especially relative to spec concepts and especially due to not knowing how it gets set and unset during a page load.

Comment 24

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

>  * Script-inserted inline scripts always run immediately (even if there are> style sheets blocking script), so there's no list for them.Is this really safe? We explicitly added the script blocking since sites were breaking without it.> They run off a> script-runner, because the insertion DOM modification has an update batch.This batch ends as soon as the <script> has been parsed, right? We can't end up inserting or closing other content after the <script> before actually running the script, right?>  * There can be only one parser-inserted script blocking at a time (when nodes> are moved between documents, bug 592366 need fixing, though). That's why> there's a single field for that instead of an array.Well, the array was also keeping track of execution order for parser and .write inserted scripts. Something that is still important, right? Hmm.. or maybe there can now only be one of these if you disregard currently executing scripts?>  * Script-inserted external scripts and parser-inserted async script work the> same way, so they go into one array.Awesome.>  * Decoupling defer scripts from parser-blocking scripts makes things> conceptually clearer and conceptually the same as in the spec.Yeah, this seems like a good idea.>  * document.written scripts don't block on style sheets blocking scripts. This> way, document.write() returns predictably instead of racing with style sheet> loads. The details are imitated from IE for now, because Hixie didn't respond> to my bug about the details in time for this beta.Like with the first bullet, this scares me a bit from a web-compat point of view.>  * If an external script is immediately available due to preloading, a trip> through the event loop is made in order not to expose event loop differences> from preloading to Web content (especially to avoid running script-inserted> scripts synchronously).Hmm.. for script-inserted scripts this does seem like a good idea. For parser inserted scripts (which I imagine is a more common case for preloaded scripts), it seems wasteful to do a event loop round trip though, right?>  * document.written scripts might now behave differently relative to the> changes made in bug 364692. I didn't understand the fix for bug 364692.Why are we in general treating document.written scripts differently from parsed ones?

Comment 25

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Not sure what happened with bugzilla there. Reposting:

>  * Script-inserted inline scripts always run immediately (even if there are
> style sheets blocking script), so there's no list for them.

Is this really safe? We explicitly added the script blocking since sites were
breaking without it.

> They run off a
> script-runner, because the insertion DOM modification has an update batch.

This batch ends as soon as the <script> has been parsed, right? We can't end up
inserting or closing other content after the <script> before actually running
the script, right?

>  * There can be only one parser-inserted script blocking at a time (when nodes
> are moved between documents, bug 592366 need fixing, though). That's why
> there's a single field for that instead of an array.

Well, the array was also keeping track of execution order for parser and .write
inserted scripts. Something that is still important, right? Hmm.. or maybe
there can now only be one of these if you disregard currently executing
scripts?

>  * Script-inserted external scripts and parser-inserted async script work the
> same way, so they go into one array.

Awesome.

>  * Decoupling defer scripts from parser-blocking scripts makes things
> conceptually clearer and conceptually the same as in the spec.

Yeah, this seems like a good idea.

>  * document.written scripts don't block on style sheets blocking scripts. This
> way, document.write() returns predictably instead of racing with style sheet
> loads. The details are imitated from IE for now, because Hixie didn't respond
> to my bug about the details in time for this beta.

Like with the first bullet, this scares me a bit from a web-compat point of
view.

>  * If an external script is immediately available due to preloading, a trip
> through the event loop is made in order not to expose event loop differences
> from preloading to Web content (especially to avoid running script-inserted
> scripts synchronously).

Hmm.. for script-inserted scripts this does seem like a good idea. For parser
inserted scripts (which I imagine is a more common case for preloaded scripts),
it seems wasteful to do a event loop round trip though, right?

>  * document.written scripts might now behave differently relative to the
> changes made in bug 364692. I didn't understand the fix for bug 364692.

Why are we in general treating document.written scripts differently from parsed
ones?



I talked with bz. It does seem that we need to wait for stylesheets even for parser-created inline scripts.

It's unclear if document.written scripts should wait or not, but it seems like a scary change to make now. Is there evidence that having document.written scripts not wait is needed or even safe?

Comment 26

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 473538 [details] [diff] [review]
Fix that passes reftest and uses PRUint8 for the field

Ok, so I think we need to undo the changes to make inline and document.written scripts not wait for stylesheets.

bz has apparently raised this issue with Hixie, that this is needed for web compat, but so far the spec has not been changed.

It does seem like there are some nice changes in here though. I would be fine with either taking a modified version of this patch, or taking a simpler patch to just make nsHTML/SVGScriptElement set mAsync to true for DOM-created elements, and take this bigger rewrite after the Firefox 4 branching. It seems like this would get us all of the behavioural changes that we want for now?

I'd love to see a bigger rewrite for how we handle script blocking/loading. Right now the element, the sink, the parser, the document and the scriptloader are all involved in keeping track of what is pending and blocking. It'd be very nice to simplify this.

Comment 27

[Henri Sivonen (:hsivonen)]

(In reply to comment #25)
> Not sure what happened with bugzilla there. Reposting:
> 
> >  * Script-inserted inline scripts always run immediately (even if there are
> > style sheets blocking script), so there's no list for them.
> 
> Is this really safe? We explicitly added the script blocking since sites were
> breaking without it.

I'll test WebKit and IE again, but this is what the spec says.

> > They run off a
> > script-runner, because the insertion DOM modification has an update batch.
> 
> This batch ends as soon as the <script> has been parsed, right? We can't end up
> inserting or closing other content after the <script> before actually running
> the script, right?

Parser-inserted scripts don't run off a script runner. Only script-inserted inline scripts.

> >  * There can be only one parser-inserted script blocking at a time (when nodes
> > are moved between documents, bug 592366 need fixing, though). That's why
> > there's a single field for that instead of an array.
> 
> Well, the array was also keeping track of execution order for parser and .write
> inserted scripts. Something that is still important, right? Hmm.. or maybe
> there can now only be one of these if you disregard currently executing
> scripts?

document.written scripts are parser-inserted, too. There can be only one script that is blocking the parser. It can be a document.written external script (blocking on getting itself loaded), an inline script from the network stream (blocking on style sheets) or an external script from the network stream (blocking on itself getting loaded and potentially also on style sheets).

> >  * document.written scripts don't block on style sheets blocking scripts. This
> > way, document.write() returns predictably instead of racing with style sheet
> > loads. The details are imitated from IE for now, because Hixie didn't respond
> > to my bug about the details in time for this beta.
> 
> Like with the first bullet, this scares me a bit from a web-compat point of
> view.

This I tested very carefully. WebKit and Opera don't block scripts on style sheets at all. IE doesn't block document.written scripts on style sheets. Since Gecko is the only engine blocking document.written scripts on style sheets, I have a hard time believing it's compat-critical to do so. OTOH, it seems bad to make the time when document.write returns depend on style sheet loads.
 
> >  * If an external script is immediately available due to preloading, a trip
> > through the event loop is made in order not to expose event loop differences
> > from preloading to Web content (especially to avoid running script-inserted
> > scripts synchronously).
> 
> Hmm.. for script-inserted scripts this does seem like a good idea. For parser
> inserted scripts (which I imagine is a more common case for preloaded scripts),
> it seems wasteful to do a event loop round trip though, right?

Maybe the cases where the page could detect this are enough of edge cases that we could get away with optimizing for perf here (in the case of scripts originating from the network stream, ie.e neither script-inserted not document.written).

> >  * document.written scripts might now behave differently relative to the
> > changes made in bug 364692. I didn't understand the fix for bug 364692.
> 
> Why are we in general treating document.written scripts differently from parsed
> ones?

Because it's bad for document.write to return at style load-dependent times. ReadyToExecuteScripts() is about that except for the changes you made in bug 364692.

> I talked with bz. It does seem that we need to wait for stylesheets even for
> parser-created inline scripts.

For scripts coming from the network stream (not document.written), sure. The patch is doing that:
+  switch (aElement->GetParserCreated()) {
+    case NS_FROM_PARSER_NETWORK:
+      if (!ReadyToExecuteScripts()) {
+        // the is a style sheet blocking scripts
+        NS_ASSERTION(!mParserBlockingRequest,
+            "There can be only one parser-blocking script at a time");
+        mParserBlockingRequest = request;
+        return NS_ERROR_HTMLPARSER_BLOCK;

(Typo in the comment: s/the/there/.)

> It's unclear if document.written scripts should wait or not, but it seems like
> a scary change to make now. Is there evidence that having document.written
> scripts not wait is needed or even safe?

No other browser waits and waiting means document.write returns at unpredictable times. Hard to tell if making document.write return predictably is needed but it sure seems bad for it to retun earlier when style sheets load slower.

Comment 28

[Henri Sivonen (:hsivonen)]

(In reply to comment #27)
> (In reply to comment #25)
> > Not sure what happened with bugzilla there. Reposting:
> > 
> > >  * Script-inserted inline scripts always run immediately (even if there are
> > > style sheets blocking script), so there's no list for them.
> > 
> > Is this really safe? We explicitly added the script blocking since sites were
> > breaking without it.
> 
> I'll test WebKit and IE again, but this is what the spec says.

I haven't tested yet, but for this situation to arise, you'd have to insert the sript-inserted script from a timeout, event handler, async script or postMessage. If you tried to insert it from a regular script, that script itself would block.

Comment 29

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to comment #27)
> (In reply to comment #25)
> > Not sure what happened with bugzilla there. Reposting:
> > 
> > >  * Script-inserted inline scripts always run immediately (even if there are
> > > style sheets blocking script), so there's no list for them.
> > 
> > Is this really safe? We explicitly added the script blocking since sites were
> > breaking without it.
> 
> I'll test WebKit and IE again, but this is what the spec says.

Ah, I forgot that this is only in relation to script inserted inline scripts, not parser inserted ones. That might be safe enough, will confer with bz.

> > >  * There can be only one parser-inserted script blocking at a time (when nodes
> > > are moved between documents, bug 592366 need fixing, though). That's why
> > > there's a single field for that instead of an array.
> > 
> > Well, the array was also keeping track of execution order for parser and .write
> > inserted scripts. Something that is still important, right? Hmm.. or maybe
> > there can now only be one of these if you disregard currently executing
> > scripts?
> 
> document.written scripts are parser-inserted, too. There can be only one script
> that is blocking the parser. It can be a document.written external script
> (blocking on getting itself loaded), an inline script from the network stream
> (blocking on style sheets) or an external script from the network stream
> (blocking on itself getting loaded and potentially also on style sheets).

Yeah, I think you're right.

> > >  * document.written scripts don't block on style sheets blocking scripts. This
> > > way, document.write() returns predictably instead of racing with style sheet
> > > loads. The details are imitated from IE for now, because Hixie didn't respond
> > > to my bug about the details in time for this beta.
> > 
> > Like with the first bullet, this scares me a bit from a web-compat point of
> > view.
> 
> This I tested very carefully. WebKit and Opera don't block scripts on style
> sheets at all. IE doesn't block document.written scripts on style sheets. Since
> Gecko is the only engine blocking document.written scripts on style sheets, I
> have a hard time believing it's compat-critical to do so. OTOH, it seems bad to
> make the time when document.write returns depend on style sheet loads.

Is IE as non-blocking on stylesheets as we are? Remember that we had to introduce this once we made stylesheets not block the parser the way <script>s do.

> > >  * If an external script is immediately available due to preloading, a trip
> > > through the event loop is made in order not to expose event loop differences
> > > from preloading to Web content (especially to avoid running script-inserted
> > > scripts synchronously).
> > 
> > Hmm.. for script-inserted scripts this does seem like a good idea. For parser
> > inserted scripts (which I imagine is a more common case for preloaded scripts),
> > it seems wasteful to do a event loop round trip though, right?
> 
> Maybe the cases where the page could detect this are enough of edge cases that
> we could get away with optimizing for perf here (in the case of scripts
> originating from the network stream, ie.e neither script-inserted not
> document.written).

That's what I was thinking. Don't feel strongly on that should be done in this bug or not.

> > >  * document.written scripts might now behave differently relative to the
> > > changes made in bug 364692. I didn't understand the fix for bug 364692.
> > 
> > Why are we in general treating document.written scripts differently from parsed
> > ones?
> 
> Because it's bad for document.write to return at style load-dependent times.
> ReadyToExecuteScripts() is about that except for the changes you made in bug
> 364692.

Note that we're not affecting document.write returning behavior, right? document.write never waits for any loads before returning, it's just a question of if it waits with inserting content into the DOM, right?

> > I talked with bz. It does seem that we need to wait for stylesheets even for
> > parser-created inline scripts.
> 
> For scripts coming from the network stream (not document.written), sure. The
> patch is doing that:
> +  switch (aElement->GetParserCreated()) {
> +    case NS_FROM_PARSER_NETWORK:
> +      if (!ReadyToExecuteScripts()) {
> +        // the is a style sheet blocking scripts
> +        NS_ASSERTION(!mParserBlockingRequest,
> +            "There can be only one parser-blocking script at a time");
> +        mParserBlockingRequest = request;
> +        return NS_ERROR_HTMLPARSER_BLOCK;
> 
> (Typo in the comment: s/the/there/.)

Yeah. Let me double-check with bz on this tomorrow.

I'd still feel more comfortable with taking a more concervative approach to the changes here. We can always check in this more clean-upy patch once we branch which should be in a matter of days. The setup we currently have is pretty fragile and I'd prefer to rewrite the whole thing (including the parts in the documents) than just clean up parts of it.

Comment 30

[Henri Sivonen (:hsivonen)]

Attachment: The interdiff to avoid fixing document.write and to avoid the event loop spin on preloaded network stream-orinating parser-inserted scripts

Comment 31

[Henri Sivonen (:hsivonen)]

(In reply to comment #29)
> (In reply to comment #27)
> > (In reply to comment #25)
> > > Not sure what happened with bugzilla there. Reposting:
> > > 
> > > >  * Script-inserted inline scripts always run immediately (even if there are
> > > > style sheets blocking script), so there's no list for them.
> > > 
> > > Is this really safe? We explicitly added the script blocking since sites were
> > > breaking without it.
> > 
> > I'll test WebKit and IE again, but this is what the spec says.
> 
> Ah, I forgot that this is only in relation to script inserted inline scripts,
> not parser inserted ones. That might be safe enough, will confer with bz.

Note that letting style sheets block script-inserted inline scripts would defeat a big part the point of this fix. The problem in bug 585620 is that in Gecko, jQuery's globalEval sometimes doesn't run synchronously but returns right away and runs the script at a later time. If we made script-inserted inline scripts block on pending sheets, jQuery's globalEval would still fail to run synchronously when there are pending sheets. Making script-inserted inline scripts run right away regardless of pending loads of any kind--be they sheets or scripts--is both HTML5-compliant and makes jQuery's globalEval predictably synchronous. (I think it's also consistent with WebKit and IE but I still haven't written the test case to be 100% sure.)

> > > >  * document.written scripts don't block on style sheets blocking scripts. This
> > > > way, document.write() returns predictably instead of racing with style sheet
> > > > loads. The details are imitated from IE for now, because Hixie didn't respond
> > > > to my bug about the details in time for this beta.
> > > 
> > > Like with the first bullet, this scares me a bit from a web-compat point of
> > > view.
> > 
> > This I tested very carefully. WebKit and Opera don't block scripts on style
> > sheets at all. IE doesn't block document.written scripts on style sheets. Since
> > Gecko is the only engine blocking document.written scripts on style sheets, I
> > have a hard time believing it's compat-critical to do so. OTOH, it seems bad to
> > make the time when document.write returns depend on style sheet loads.
> 
> Is IE as non-blocking on stylesheets as we are? 

In IE, parser-inserted scripts that originate from parsing a script tag in the network stream of HTML source block if there are pending sheets. Parser-inserted scripts that originate from parsing a document.written script tag don't block on pending sheets in IE.

In Opera and WebKit, parser-inserted scripts don't block on pending sheets.

> Remember that we had to
> introduce this once we made stylesheets not block the parser the way <script>s
> do.

Are you sure the compat constraint wasn't only that scripts that appear in the document source (not document.written) need to block?

> > > >  * document.written scripts might now behave differently relative to the
> > > > changes made in bug 364692. I didn't understand the fix for bug 364692.
> > > 
> > > Why are we in general treating document.written scripts differently from parsed
> > > ones?
> > 
> > Because it's bad for document.write to return at style load-dependent times.
> > ReadyToExecuteScripts() is about that except for the changes you made in bug
> > 364692.
> 
> Note that we're not affecting document.write returning behavior, right?
> document.write never waits for any loads before returning, it's just a question
> of if it waits with inserting content into the DOM, right?

The case I'm talking about is document.write("<script>foo();</script><div></div>"); returning before the div has been parsed and inserted to the DOM if there are pending sheet loads. I filed bug 596942 for this to avoid delaying this beta7 blocker on this aspect of the r-'ed patch.

document.write("<script src="..."></script><div></div>"); always returns before the div has been parsed and inserted into the DOM, so that's not the case I'm talking about.
> I'd still feel more comfortable with taking a more concervative approach to the
> changes here. We can always check in this more clean-upy patch once we branch
> which should be in a matter of days. The setup we currently have is pretty
> fragile and I'd prefer to rewrite the whole thing (including the parts in the
> documents) than just clean up parts of it.

It seemed to me that it's easier to see that the patch does the HTML5-compliant thing if it's written so that you can follow the code to see how it relates to the spec. Hacking the old code to retain the old data structures, etc., could yield the same behavior, but it might not be clear why such a hack yielded the right result.

I can't tell how strict your comment is. Do you mean I should start writing a whole new patch that changes the old code minimally and the whole approach here is r- for beta 7? Or is there a chance of r+ on this general approach if leave document.write as doing the old (racy) thing here?

Comment 32

[Henri Sivonen (:hsivonen)]

Attachment: Fix only the problem from the bug summary without fixing document.write

This patch comments out the document.write() fix (for easy restoration in the follow-up bug). This patch also avoids the event loop trip in the case of preloaded scripts that originate from parsing a tag in the network stream. (However, avoiding the event loop trip in the other preloaded cases would be blatantly wrong, so I didn't remove the event loop trip for those.)

Comment 33

[Henri Sivonen (:hsivonen)]

(The latest patch qfolds the interdiff patch into the previously r-'ed patch. No other changes.)

Comment 34

[Henri Sivonen (:hsivonen)]

http://hsivonen.iki.fi/test/moz/sheet-blocking-script4.php shows that WebKit and Opera don't block script-inserted inline scripts on pending sheets.

In IE9 beta when the timeout fires, document.getElementsByTagName("p")[0] is undefined.

Comment 35

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 475815 [details] [diff] [review]
Fix only the problem from the bug summary without fixing document.write

>@@ -510,145 +531,149 @@ nsScriptLoader::ProcessScriptElement(nsI
...
>+      // XXX what if the charset attribute of the element and the charset
>+      // of the preload don't match?

We should just toss out the preload and fall back to the normal code path. Can you file a bug and nominate it as blocker?

>+    PRBool async = !aElement->GetParserCreated() || aElement->GetScriptAsync();
>+
>+    // we now have a request that may or may not be still loading
>+    if (!async && aElement->GetScriptDeferred()) {
>+      // We don't want to run this yet.
>+      // XXX assert that the parse is still ongoing
>+      mDeferRequests.AppendObject(request);
>+      return NS_OK;
>+    }

I take it that the GetParserCreated check is what prevents us from falling in here if parsing is done? If so, could you add a comment to that extent?

In any case, please do add the assertion.


>+  switch (aElement->GetParserCreated()) {
>+    case NS_FROM_PARSER_NETWORK:
>+    case NS_FROM_PARSER_DOCUMENT_WRITE: // XXX remove this line as bug 596942

Please don't use switch statements with fall-throughs. It makes for very hard to follow code and there is really no reason to optimize to that level here (and it's likely what it'll compile to anyway here). Simply use if-statements. (This also lets you remove the NS_NOTREACHED at the end of the function)

I talked it over with bz and we agreed that it's probably safe enough to allow document.written stuff not block on stylesheets.


> void
> nsScriptLoader::ProcessPendingRequests()
> {
>-  while (1) {
>-    nsRefPtr<nsScriptLoadRequest> request;
>-    if (ReadyToExecuteScripts()) {
>-      request = GetFirstPendingRequest();
>-      if (request && !request->mLoading) {
>-        mRequests.RemoveObject(request);
>+  nsRefPtr<nsScriptLoadRequest> request;
>+  if (mParserBlockingRequest &&
>+      (!mParserBlockingRequest->mLoading ||
>+        mParserBlockingRequest->mIsInline) &&

The last line above is wrongly indented, should line up with the '!'. Though is the mIsInline check here really needed? Don't all inline scripts have mLoading=false?

>+      ReadyToExecuteScripts()) {
>+    request = mParserBlockingRequest;
>+    mParserBlockingRequest = nsnull;

request = mParserBlockingRequest.forget();
or
request.swap(mParserBlockingRequest);


>+  PRInt32 i = 0;
>+  while (mEnabled && i < mAsyncRequests.Count()) {
>+    if (!mAsyncRequests[i]->mLoading) {
>+      request = mAsyncRequests[i];

You can use .forget() here too to save a few of cycles of refcounting. Same in the defer loop.

>+  if (mShouldRunDeferScriptsNow) {
>+    i = 0;
>+    while (i < mDeferRequests.Count()) {
>+      if (!mDeferRequests[i]->mLoading) {
>+        request = mDeferRequests[i];
>+        mDeferRequests.RemoveObjectAt(i);
>+        ProcessRequest(request);
>+        continue;
>       }

This doesn't appear to be correct. We want to run defer scripts in order, no? IIRC that is what IE does and what we've done as long as we've supported defer.

>-  if (mUnblockOnloadWhenDoneProcessing && mDocument &&
>-      !GetFirstPendingRequest() && !mAsyncRequests.Count()) {
>+  if (mShouldRunDeferScriptsNow && mDocument &&

Given that mShouldRunDeferScriptsNow is used for more than just running defer scripts, something like mDocumentParsingDone might be a better name.


>@@ -550,17 +550,17 @@ nsHTMLScriptElement::MaybeProcessScript(
> {
>   nsresult rv = nsScriptElement::MaybeProcessScript();
>   if (rv == NS_CONTENT_SCRIPT_IS_EVENTHANDLER) {
>     // Don't return NS_CONTENT_SCRIPT_IS_EVENTHANDLER since callers can't deal
>     rv = NS_OK;
> 
>     // We tried to evaluate the script but realized it was an eventhandler
>     // mEvaluated will already be set at this point
>-    NS_ASSERTION(mIsEvaluated, "should have set mIsEvaluated already");
>+    NS_ASSERTION(mAlreadyStarted, "should have set mIsEvaluated already");

Adjust the comment too.

>diff --git a/dom/tests/mochitest/bugs/child_bug260264.html b/dom/tests/mochitest/bugs/child_bug260264.html
>diff --git a/dom/tests/mochitest/bugs/grandchild_bug260264.html b/dom/tests/mochitest/bugs/grandchild_bug260264.html
>diff --git a/dom/tests/mochitest/bugs/test_bug260264.html b/dom/tests/mochitest/bugs/test_bug260264.html
>diff --git a/dom/tests/mochitest/bugs/utils_bug260264.js b/dom/tests/mochitest/bugs/utils_bug260264.js

Why are the changes to these tests needed?

r=me with those things fixed.

Comment 36

[Henri Sivonen (:hsivonen)]

(In reply to comment #35)
> Comment on attachment 475815 [details] [diff] [review]
> Fix only the problem from the bug summary without fixing document.write
> 
> >@@ -510,145 +531,149 @@ nsScriptLoader::ProcessScriptElement(nsI
> ...
> >+      // XXX what if the charset attribute of the element and the charset
> >+      // of the preload don't match?
> 
> We should just toss out the preload and fall back to the normal code path. Can
> you file a bug and nominate it as blocker?

Bug 597345.

> >+    PRBool async = !aElement->GetParserCreated() || aElement->GetScriptAsync();
> >+
> >+    // we now have a request that may or may not be still loading
> >+    if (!async && aElement->GetScriptDeferred()) {
> >+      // We don't want to run this yet.
> >+      // XXX assert that the parse is still ongoing
> >+      mDeferRequests.AppendObject(request);
> >+      return NS_OK;
> >+    }
> 
> I take it that the GetParserCreated check is what prevents us from falling in
> here if parsing is done? 

Yes.

> If so, could you add a comment to that extent?

Added.

> In any case, please do add the assertion.

Added. It'll fire on bug 592366, but that's appropriate, since that's a bug.

> >+  switch (aElement->GetParserCreated()) {
> >+    case NS_FROM_PARSER_NETWORK:
> >+    case NS_FROM_PARSER_DOCUMENT_WRITE: // XXX remove this line as bug 596942
> 
> Please don't use switch statements with fall-throughs. It makes for very hard
> to follow code and there is really no reason to optimize to that level here
> (and it's likely what it'll compile to anyway here). Simply use if-statements.
> (This also lets you remove the NS_NOTREACHED at the end of the function)

OK.

> I talked it over with bz and we agreed that it's probably safe enough to allow
> document.written stuff not block on stylesheets.

Restored.

> > void
> > nsScriptLoader::ProcessPendingRequests()
> > {
> >-  while (1) {
> >-    nsRefPtr<nsScriptLoadRequest> request;
> >-    if (ReadyToExecuteScripts()) {
> >-      request = GetFirstPendingRequest();
> >-      if (request && !request->mLoading) {
> >-        mRequests.RemoveObject(request);
> >+  nsRefPtr<nsScriptLoadRequest> request;
> >+  if (mParserBlockingRequest &&
> >+      (!mParserBlockingRequest->mLoading ||
> >+        mParserBlockingRequest->mIsInline) &&
> 
> The last line above is wrongly indented, should line up with the '!'. Though is
> the mIsInline check here really needed? Don't all inline scripts have
> mLoading=false?

Removed.

> >+      ReadyToExecuteScripts()) {
> >+    request = mParserBlockingRequest;
> >+    mParserBlockingRequest = nsnull;
> 
> request = mParserBlockingRequest.forget();
> or
> request.swap(mParserBlockingRequest);

Used swap. Had to switch from nsRefPtr to nsCOMPtr for it to work.

> >+  PRInt32 i = 0;
> >+  while (mEnabled && i < mAsyncRequests.Count()) {
> >+    if (!mAsyncRequests[i]->mLoading) {
> >+      request = mAsyncRequests[i];
> 
> You can use .forget() here too to save a few of cycles of refcounting. Same in
> the defer loop.

Can't do that, because indexing into an nsCOMArray returns a plain pointer. Maybe we should switch from nsCOMArray<Foo> to nsTArray<nsCOMPtr<Foo>> some time.

> >+  if (mShouldRunDeferScriptsNow) {
> >+    i = 0;
> >+    while (i < mDeferRequests.Count()) {
> >+      if (!mDeferRequests[i]->mLoading) {
> >+        request = mDeferRequests[i];
> >+        mDeferRequests.RemoveObjectAt(i);
> >+        ProcessRequest(request);
> >+        continue;
> >       }
> 
> This doesn't appear to be correct. We want to run defer scripts in order, no?
> IIRC that is what IE does and what we've done as long as we've supported defer.

Fixed.

> >-  if (mUnblockOnloadWhenDoneProcessing && mDocument &&
> >-      !GetFirstPendingRequest() && !mAsyncRequests.Count()) {
> >+  if (mShouldRunDeferScriptsNow && mDocument &&
> 
> Given that mShouldRunDeferScriptsNow is used for more than just running defer
> scripts, something like mDocumentParsingDone might be a better name.

Renamed.

> >@@ -550,17 +550,17 @@ nsHTMLScriptElement::MaybeProcessScript(
> > {
> >   nsresult rv = nsScriptElement::MaybeProcessScript();
> >   if (rv == NS_CONTENT_SCRIPT_IS_EVENTHANDLER) {
> >     // Don't return NS_CONTENT_SCRIPT_IS_EVENTHANDLER since callers can't deal
> >     rv = NS_OK;
> > 
> >     // We tried to evaluate the script but realized it was an eventhandler
> >     // mEvaluated will already be set at this point
> >-    NS_ASSERTION(mIsEvaluated, "should have set mIsEvaluated already");
> >+    NS_ASSERTION(mAlreadyStarted, "should have set mIsEvaluated already");
> 
> Adjust the comment too.
> 
> >diff --git a/dom/tests/mochitest/bugs/child_bug260264.html b/dom/tests/mochitest/bugs/child_bug260264.html
> >diff --git a/dom/tests/mochitest/bugs/grandchild_bug260264.html b/dom/tests/mochitest/bugs/grandchild_bug260264.html
> >diff --git a/dom/tests/mochitest/bugs/test_bug260264.html b/dom/tests/mochitest/bugs/test_bug260264.html
> >diff --git a/dom/tests/mochitest/bugs/utils_bug260264.js b/dom/tests/mochitest/bugs/utils_bug260264.js
> 
> Why are the changes to these tests needed?

The .js file relied script-inserted external scripts blocking other scripts.

> r=me with those things fixed.

Thanks.

Landed: http://hg.mozilla.org/mozilla-central/rev/0ab712643a66

Comment 37

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to comment #36)

> > request = mParserBlockingRequest.forget();
> > or
> > request.swap(mParserBlockingRequest);
> 
> Used swap. Had to switch from nsRefPtr to nsCOMPtr for it to work.

Strange. What error did nsRefPtr.forget() give you?

nsCOMPtr isn't really appropriate here since you're not holding on to an interface.

> > >+  PRInt32 i = 0;
> > >+  while (mEnabled && i < mAsyncRequests.Count()) {
> > >+    if (!mAsyncRequests[i]->mLoading) {
> > >+      request = mAsyncRequests[i];
> > 
> > You can use .forget() here too to save a few of cycles of refcounting. Same in
> > the defer loop.
> 
> Can't do that, because indexing into an nsCOMArray returns a plain pointer.
> Maybe we should switch from nsCOMArray<Foo> to nsTArray<nsCOMPtr<Foo>> some
> time.

Yeah, I keep thinking in terms of nsTArray most of the time these days. nsCOMArray isn't really approprate here since we're not holding on to an interface.

Comment 38

[Henri Sivonen (:hsivonen)]

(In reply to comment #37)
> (In reply to comment #36)
> 
> > > request = mParserBlockingRequest.forget();
> > > or
> > > request.swap(mParserBlockingRequest);
> > 
> > Used swap. Had to switch from nsRefPtr to nsCOMPtr for it to work.
> 
> Strange. What error did nsRefPtr.forget() give you?

It was .swap().

No matching function to call for 'nsRefPtr<nsScriptLoadRequest>::swap(nsCOMPtr<nsScriptLoadRequest>&).

The other nsCOMPtr happened because the old code used an nsCOMArray...

> nsCOMPtr isn't really appropriate here since you're not holding on to an
> interface.
> 
> > > >+  PRInt32 i = 0;
> > > >+  while (mEnabled && i < mAsyncRequests.Count()) {
> > > >+    if (!mAsyncRequests[i]->mLoading) {
> > > >+      request = mAsyncRequests[i];
> > > 
> > > You can use .forget() here too to save a few of cycles of refcounting. Same in
> > > the defer loop.
> > 
> > Can't do that, because indexing into an nsCOMArray returns a plain pointer.
> > Maybe we should switch from nsCOMArray<Foo> to nsTArray<nsCOMPtr<Foo>> some
> > time.
> 
> Yeah, I keep thinking in terms of nsTArray most of the time these days.
> nsCOMArray isn't really approprate here since we're not holding on to an
> interface.

Filed bug 597368.

Comment 39

[Henri Sivonen (:hsivonen)]

A bot says (see bug 531056) that this push regressed Dromaeo. Why would running scripts sooner make Dromaeo slower?

Comment 40

[Henri Sivonen (:hsivonen)]

Since this bug made jQuery's globalEval evaluate its script synchronously, is it possible that a JS lib test has been testing jQuery in a way that called globalEval while there were pending script or sheet loads, globalEval returned, the timing ended and then the script passed to globalEval gets ran outside the measurements?

Comment 41

[Henri Sivonen (:hsivonen)]

jresig, any ideas why making sript-inserted inline scripts run synchronously and script-inserted external scripts run like async scripts would regress Dromaeo? Does my wild guess in comment 40 look like a possible explanation?

Comment 42

[Mozilla RelEng Bot]

A changeset from this bug was associated with a Dromaeo (CSS) regression. boo-urns :(

  Previous: avg 2081.901 stddev 35.646 of 30 runs up to 268ef4ccb5ff
  New     : avg 1986.498 stddev 14.409 of 5 runs since bc15c280c430
  Change  : -95.403 (-4.58% / z=2.676)
  Graph   : http://mzl.la/ba6Hsw

The regression occurred from changesets in the following range:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=268ef4ccb5ff&tochange=bc15c280c430

The tag [suspect-regress-dromaeo_css] has been added to the status whiteboard;
please remove it only once you have confirmed this bug is not the cause
of the regression.

Comment 43

[Mozilla RelEng Bot]

A changeset from this bug was associated with a Dromaeo (DOM) regression. boo-urns :(

  Previous: avg 246.591 stddev 2.979 of 30 runs up to 268ef4ccb5ff
  New     : avg 233.803 stddev 2.192 of 5 runs since bc15c280c430
  Change  : -12.788 (-5.19% / z=4.292)
  Graph   : http://mzl.la/brFt7U

The regression occurred from changesets in the following range:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=268ef4ccb5ff&tochange=bc15c280c430

The tag [suspect-regress-dromaeo_dom] has been added to the status whiteboard;
please remove it only once you have confirmed this bug is not the cause
of the regression.

Comment 44

[Mozilla RelEng Bot]

A changeset from this bug was associated with a Dromaeo (jslib) regression. boo-urns :(

  Previous: avg 130.889 stddev 2.485 of 30 runs up to 268ef4ccb5ff
  New     : avg 121.915 stddev 0.830 of 5 runs since bc15c280c430
  Change  : -8.975 (-6.86% / z=3.612)
  Graph   : http://mzl.la/bVrIoj

The regression occurred from changesets in the following range:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=268ef4ccb5ff&tochange=bc15c280c430

The tag [suspect-regress-dromaeo_jslib] has been added to the status whiteboard;
please remove it only once you have confirmed this bug is not the cause
of the regression.

Comment 45

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Henri, there have been added automated tests. Are those sufficient so we don't need manual testing?

Comment 46

[Boris Zbarsky [:bzbarsky]]

Henri, can you reproduce the Dromaeo regression locally?

Comment 47

[Henri Sivonen (:hsivonen)]

(In reply to comment #46)
> Henri, can you reproduce the Dromaeo regression locally?

I can't using the Dromaeo Web harness. My guess is that this bug didn't actually regress any of the Dromaeo test payload but regressed the Talos Dromaeo harness.

As Murphy would have it, I haven't gotten the Talos Dromaeo harness running locally: My existing Standalone Talos 1.6 no longer manages to control Minefield and CVS talos and Standalone Talos 1.8 have unsatisfied dependencies (pageloader.xpi and ffprocess_linux respectively). Googling for the latter now.

My hypothesis from comment 40 appears to be incorrect based on a printf that'd fire when an inline script gets blocked in the old version of nsScriptLoader not firing on the try server.

My new hypothesis is that since mEnabled affects the evaluation time of all async scripts, it is now adversely affecting script-inserted external scripts. I'm testing this hypothesis.

Comment 48

[Henri Sivonen (:hsivonen)]

Aargh. The Ubuntu Archive Manager had failed to extract Standalone Talos 1.8 correctly. Using command line unzip works.

Comment 49

[Henri Sivonen (:hsivonen)]

Attachment: The only Gecko-side adjustment I can think of that wouldn't regress reftest and wouldn't regress HTML5-compliance of cross-browser compatibility

Comment 50

[Henri Sivonen (:hsivonen)]

(In reply to comment #45)
> Henri, there have been added automated tests. Are those sufficient so we don't
> need manual testing?

I think the automated tests are sufficient, although it would be even better to adapt
http://hsivonen.iki.fi/test/moz/sheet-blocking-script.html
http://hsivonen.iki.fi/test/moz/sheet-blocking-script2.html
http://hsivonen.iki.fi/test/moz/sheet-blocking-script3.html
http://hsivonen.iki.fi/test/moz/sheet-blocking-script-baseline.php
into mochitests, too, in due course.

I think there isn't any specific manual testing to do here that wouldn't make more sense to write as additional mochitests.

Comment 51

[Boris Zbarsky [:bzbarsky]]

Er.... Talos doesn't run Dromaeo in the normal web harness??

Comment 52

[Henri Sivonen (:hsivonen)]

(In reply to comment #51)
> Er.... Talos doesn't run Dromaeo in the normal web harness??

In the Web harness, there's no navigation: all the tests are on one page. On Talos, each test has a dedicated page.

I'll back the patch out tomorrow per sicking's email to dev-tree-management (unless someone else backs it out sooner, of course). :-(

Comment 53

[Boris Zbarsky [:bzbarsky]]

Ugh. Can we file a bug on talos to run the tests in the web harness?  The results are _very_ different from one-test-per-page in some cases, as I recall.  :(

Comment 54

[Henri Sivonen (:hsivonen)]

Now things are getting *really* weird:
http://groups.google.com/group/mozilla.dev.tree-management/browse_thread/thread/ec0941b6f50a775c#

Comment 55

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Looks like sicking backed this out:
http://hg.mozilla.org/mozilla-central/rev/2b4ecab1e93a

Comment 56

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yes, I would say go ahead and reland this.

Comment 57

[Henri Sivonen (:hsivonen)]

ETA for re-landing: Wednesday morning Europe time.

Comment 58

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Checked in again. Henri, please keep an eye on the tree again, though not really sure what to do if it would regress...

http://hg.mozilla.org/mozilla-central/rev/4991e79cbc3e

Comment 59

[Henri Sivonen (:hsivonen)]

(In reply to comment #58)
> Checked in again.

Thank you.

> Henri, please keep an eye on the tree again, though not
> really sure what to do if it would regress...

This time, there was only
http://groups.google.com/group/mozilla.dev.tree-management/browse_thread/thread/d759fab79ff5153e#
but that one didn't happened the last time round, so I'm going to assume it was the other patch in the range.

Comment 60

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I backed this out again in order to fix bug 599583 :(

http://hg.mozilla.org/mozilla-central/rev/c0852588831b

Comment 61

[Boris Zbarsky [:bzbarsky]]

And fwiw, this time Dromaeo CSS and jslib showed improvements on the backout.  May still be PGO noise, though.

Comment 62

[Henri Sivonen (:hsivonen)]

Relanding this depends on the patch attached to bug 599583.

Comment 63

[Henri Sivonen (:hsivonen)]

Relanded: http://hg.mozilla.org/mozilla-central/rev/a60414d076b5

Comment 64

[Eric Shepherd [:sheppy]]

Added a note here:

https://developer.mozilla.org/En/HTML/Element/Script

Comment 65

[Kyle]

Just for the record and posterity sake:

http://blog.getify.com/2010/10/ff4-script-loaders-and-order-preservation/

:(

Comment 66

[Henri Sivonen (:hsivonen)]

(In reply to comment #65)
> Just for the record and posterity sake:
> 
> http://blog.getify.com/2010/10/ff4-script-loaders-and-order-preservation/
> 
> :(

Comment posted:
http://blog.getify.com/2010/10/ff4-script-loaders-and-order-preservation/#comment-748
(Currently awaiting moderation.)

Comment 67

[Kyle]

Two blog posts to respond to that comment:

http://blog.getify.com/2010/10/mozilla-labjs-the-story-unfolds/

http://blog.getify.com/2010/10/mozilla-labjs-part-2/

Comment 68

[Kyle]

BTW, the discussion has moved from the W3C mailing list to:

http://wiki.whatwg.org/wiki/Dynamic_Script_Execution_Order