Bug 8852

Created attachment 8241 [details] test case Should be: 1st alert: 0: [object CSSStyleSheet] 1: undefined 2nd alert: 0: [object CSSStyleSheet] 1: [object CSSStyleSheet] You may need to reload the page or restart Safari to reproduce the problem more than once.

(In reply to comment #0) > the initialization script fails early with an assertion. Oops, this should have been "the initialization script fails early with an exception".

> Scripts should not be executed before preceding subresources are loaded Is that specified somewhere? If not, what rules would you suggest following?

(In reply to comment #3) > Is that specified somewhere? I'm not aware of any such specification (or what specification could possibly talk about such things). > If not, what rules would you suggest following? Defining the rules would be a major step towards resolving this issue :). As a guess to ignite a discussion: perhaps a script should be paused when it accesses DOM for something that causes layout (like offsetTop does), and stylesheets are not loaded yet? And of course, when it accesses stylesheets themselves. If I understood Maciej correctly, JSC currently lacks the ability to pause script execution, though.

(In reply to comment #4) > perhaps a script should be paused when it > accesses DOM for something that causes layout (like offsetTop does), and > stylesheets are not loaded yet? And of course, when it accesses stylesheets > themselves. OK, one thing I was wondering about was what "subresources" encompassed. Is it limited to stylesheets? The other thing was "preceding" - what if the script inserts a <link rel="stylesheet">? Should it block until it's loaded? Finally, if the desired behavior is not specified, could there be a way to apply it as a quirk only to pages that may actually need it (those whose authors haven't heard of onload)? > If I understood Maciej correctly, JSC currently lacks the ability to pause > script execution, though. Especially given that, it would be very sad to not even start executing any script until stylesheets have loaded, although that does seem to be exactly what Firfox does :-(

What Mozilla does is a long-standing bug that they intend to fix: https://bugzilla.mozilla.org/show_bug.cgi?id=84582 There, I suggest blocking when trying to evaluate a <script> until the short timer has blown or the styles heets have been loaded. I wouldn't bother doing any special about inline event handlers, or scripts attached from other documents, or anything like that. They can just live in the not-yet-styled world if they are fired early. People who want to reliably do offsetTop nonsense in esoteric cases like that can use the onload event. As far as accessing style sheets that haven't loaded yet, i.e. this bug, I think there's not much one can do. That's what the "load" event is for; if the load hasn't completed yet then trying to access the style sheets isn't going to work. In short, I think this is WONTFIX or INVALID.

Created attachment 77623 [details] patch

Firefox (3.5) also blocks inline scripts inserted by document.write() which this patch doesn't do. That behavior gives a rather strange script execution order.

*** Bug 24898 has been marked as a duplicate of this bug. ***

<rdar://problem/5471785>

What does HTML5 say now?

(In reply to comment #11) > What does HTML5 say now? "If the element does not have a src attribute, and the element has been flagged as "parser-inserted", and the Document of the HTML parser or XML parser that created the script element has a style sheet that is blocking scripts: The element is the pending parsing-blocking script of the Document of the parser that created the element. (There can only be one such script per Document at a time.)" http://www.whatwg.org/specs/web-apps/current-work/multipage/scripting-1.html#script

> If the element does not have a src attribute But this bug is about a link element with an href attribute, not a script element without src? Please don't get me wrong, I'm not opposing this change myself, but there seems to be significant controversy around it. I'm also not sure what Mozilla did in <https://bugzilla.mozilla.org/show_bug.cgi?id=84582> in the end.

Comment on attachment 77623 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=77623&action=review > WebCore/dom/PendingScript.h:95 > + CachedResourceHandle<CachedScript> m_cachedScript; The patch adds a trailing space to this line, which made the source code control system think we modified it. > WebCore/html/parser/HTMLScriptRunner.cpp:-310 > - // FIXME: We do not block inline <script> tags on stylesheets to match the > - // old parser for now. When we do, the ASSERT below should be added. > - // See https://bugs.webkit.org/show_bug.cgi?id=40047 > - // ASSERT(document()->haveStylesheetsLoaded()); Why didn’t you add the assertion the way the comment said we would?

(In reply to comment #13) > > If the element does not have a src attribute > > But this bug is about a link element with an href attribute, not a script element without src? > > Please don't get me wrong, I'm not opposing this change myself, but there seems to be significant controversy around it. I'm also not sure what Mozilla did in <https://bugzilla.mozilla.org/show_bug.cgi?id=84582> in the end. I didn’t see this comment about controversy when I reviewed the patch. Please don’t consider my review an attempt to override Alexey’s concerns.

(In reply to comment #13) > > If the element does not have a src attribute > > But this bug is about a link element with an href attribute, not a script element without src? We implemented blocking external script execution on stylesheet loads years ago. Using this bug for the remaining work seems appropriate. Your test case is about inline scripts too. > Please don't get me wrong, I'm not opposing this change myself, but there seems to be significant controversy around it. I'm also not sure what Mozilla did in <https://bugzilla.mozilla.org/show_bug.cgi?id=84582> in the end. What controversy? The only reason we didn't do this along with the original change was that it was much more work with the old parser and there was other stuff to do. Having this kind of behavior difference between inline and external scripts makes no sense at all (besides, HTML5 requires it and no other engine behaves like this as far as I know). I think the unclear part is what to do with script inserted external stylesheet/inline script pair. The patch maintains the old behavior in this rare case for now even though HTML5 (or Gecko) apparently do not have this special case. https://bugs.webkit.org/show_bug.cgi?id=40047 had some discussion...

> > WebCore/html/parser/HTMLScriptRunner.cpp:-310 > > - // FIXME: We do not block inline <script> tags on stylesheets to match the > > - // old parser for now. When we do, the ASSERT below should be added. > > - // See https://bugs.webkit.org/show_bug.cgi?id=40047 > > - // ASSERT(document()->haveStylesheetsLoaded()); > > Why didn’t you add the assertion the way the comment said we would? Because the assert would be wrong as we still allow script execution with pending sheets in the case of nested script execution. The comment above tries to explain. (I could modify the assert, but asserting for the exact same thing I test with if() above seems pointless)

> What controversy? Please see comment 6 from Ian: "I think this is WONTFIX or INVALID". Mitz seemed unhappy about this in comment 5. And some comments in the Mozilla bug talk about crazy things like having a short timeout - as mentioned before, I'm not sure what they ended up doing.

(In reply to comment #18) > > What controversy? > > Please see comment 6 from Ian: "I think this is WONTFIX or INVALID". Mitz seemed unhappy about this in comment 5. And some comments in the Mozilla bug talk about crazy things like having a short timeout - as mentioned before, I'm not sure what they ended up doing. That comment is from 2006!

Alexey, I don't know how to address your concern. Perhaps you could suggest a way to move forward?

I suggest addressing comments from 2006 (perhaps reaching out to people who made them, and to Hyatt, who made some comments on IRC, if I remember correctly). Figuring out what exactly Mozilla did seems important, too.

(In reply to comment #21) > I suggest addressing comments from 2006 (perhaps reaching out to people who made them, and to Hyatt, who made some comments on IRC, if I remember correctly). Figuring out what exactly Mozilla did seems important, too. Hyatt was unhappy I didn't do this change right away when I made external scripts block on stylesheet (since it is a bad inconsistency). Ian's current position is probably best expressed by the HTML5 spec. Dan is cc'd and can comment. I have tested Mozilla 3.5 behavior and it is more aggressive than this patch (pending stylesheets block script execution in all cases, including nested document.write()).

http://trac.webkit.org/changeset/33544 was the change that implemented most of this bug (making external script execution block on stylesheets)

...where the ChangeLog says "This will make external script execution block on external stylesheet loads. A full fix (as discussed with Hyatt) will also need to block inline script execution."

Did you test Firefox 4.0pre? Behavior of Firefox 3.x is irrelevant at this point. If you are unwilling to read <https://bugzilla.mozilla.org/show_bug.cgi?id=84582>, that just means that a reviewer has to do that for you.

> Did you test Firefox 4.0pre? Behavior of Firefox 3.x is irrelevant at this point. No, as this seemed like an unlikely thing to regress. I can certainly download it and test. > If you are unwilling to read <https://bugzilla.mozilla.org/show_bug.cgi?id=84582>, that just means that a reviewer has to do that for you. I didn't manage extract any relevant information out of that bug, closed in early 2007. Perhaps you can? I would be more interested about technical comments. What kind of problems do you expect from this change?

There are several aspects here (as in every other bug): 1) Is the patch moving in the right direction? In this case, it almost certainly is. 2) Are there any minor details we'd rather not overlook? Unclear - there was a Mozilla bug that was allegedly supposed to make Mozilla behave like WebKit, and it's marked as fixed, but Mozilla still doesn't behave like WebKit in your testing. 3) Does the patch accurately implement what it claims to? That's up to reviewer to assess. 4) Is regression testing adequate? Test coverage seems good here. 5) Is there a clear history trail to follow if this causes regressions, or if someone just claims that this was a bad idea? I think that there is much more of that now as a result of our discussion. A reviewer who actually reads the patch can make more technical comments. My primary interest here is making sure that investigation was reasonably complete and that there is a history trail. In no way am I going to be a gatekeeper.

Firefox 4 beta8 behaves exactly the same as this patch in the included tests. This means they have changed the behavior (compared to ff3.5) to not block the nested script execution on stylesheets.

Created attachment 77713 [details] modified the link element fix slightly to match the same test elsewhere (test for document changes too)

Comment on attachment 77713 [details] modified the link element fix slightly to match the same test elsewhere (test for document changes too) r=me

Comment on attachment 77713 [details] modified the link element fix slightly to match the same test elsewhere (test for document changes too) View in context: https://bugs.webkit.org/attachment.cgi?id=77713&action=review > LayoutTests/platform/mac/fast/repaint/renderer-destruction-by-invalidateSelection-crash-expected.txt:17 > text run at (0,0) width 4: " " > RenderText {#text} at (0,0) size 0x0 > RenderBlock {DIV} at (0,41) size 784x0 > - RenderBlock (anonymous) at (0,41) size 784x0 > - RenderText {#text} at (0,0) size 0x0 > - RenderText {#text} at (0,0) size 0x0 > layer at (13,13) size 119x13 > RenderBlock {DIV} at (3,3) size 119x13 > caret: position 0 of child 0 {DIV} of child 1 {INPUT} of child 1 {DIV} of body > The pixel result of this test has been changed. Is it expected? The body background color is gray on Mac.

Yeah, this is expected. The test calls layoutTestContoller.display() which marks non-updated area with grey color. With this patch the stylesheet arrives before the script runs and the document is fully displayed already at that point. Updated results in http://trac.webkit.org/changeset/75057

(In reply to comment #33) > Yeah, this is expected. The test calls layoutTestContoller.display() which marks non-updated area with grey color. With this patch the stylesheet arrives before the script runs and the document is fully displayed already at that point. > > Updated results in http://trac.webkit.org/changeset/75057 ok, thanks!

FYI, this change caused a 15% regression in the initial load time of Google Docs (as they measure for users in the real-world). Since the old behavior caused compat problems, that obviously trumps performance. I'm working with them to rearrange things to be more efficient again, but this is likely to have hit other sites as well so I just wanted to add a note to the bug.

(In reply to comment #35) > FYI, this change caused a 15% regression in the initial load time of Google Docs (as they measure for users in the real-world). Since the old behavior caused compat problems, that obviously trumps performance. Similar sized regressions are showing up on more Google properties. I've filed https://bugs.webkit.org/show_bug.cgi?id=56842.