RESOLVED FIXED 8852
Scripts should not be executed before preceding stylesheets are loaded
https://bugs.webkit.org/show_bug.cgi?id=8852
Summary Scripts should not be executed before preceding stylesheets are loaded
Alexey Proskuryakov
Reported 2006-05-11 09:57:56 PDT
Steps to reproduce: 1. Open <http://www.worldofwarcraft.com/realmstatus/>. 2. Look at the JavaScript console - lots of error messages. Most likely, something in this site actually doesn't work, I haven't investigated. But the errors are scary enough on their own - the main problem here is that the initialization script fails early with an assertion. This init script tries to reference a stylesheet that is supposed to be loaded before it. However, WebKit defers loading the stylesheet, so it is not present, so we get an exception.
Attachments
test case (322 bytes, text/html)
2006-05-11 10:02 PDT, Alexey Proskuryakov
no flags
patch (10.67 KB, patch)
2010-12-29 09:29 PST, Antti Koivisto
no flags
modified the link element fix slightly to match the same test elsewhere (test for document changes too) (10.82 KB, patch)
2010-12-31 04:23 PST, Antti Koivisto
hyatt: review+
Alexey Proskuryakov
Comment 1 2006-05-11 10:02:51 PDT
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.
Alexey Proskuryakov
Comment 2 2006-05-11 11:37:47 PDT
(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".
mitz
Comment 3 2006-07-11 04:59:46 PDT
> Scripts should not be executed before preceding subresources are loaded Is that specified somewhere? If not, what rules would you suggest following?
Alexey Proskuryakov
Comment 4 2006-07-11 05:55:01 PDT
(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.
mitz
Comment 5 2006-07-11 06:28:36 PDT
(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 :-(
Ian 'Hixie' Hickson
Comment 6 2006-07-20 16:01:10 PDT
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.
Antti Koivisto
Comment 7 2010-12-29 09:29:12 PST
Antti Koivisto
Comment 8 2010-12-29 09:33:33 PST
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.
Antti Koivisto
Comment 9 2010-12-29 09:35:29 PST
*** Bug 24898 has been marked as a duplicate of this bug. ***
Antti Koivisto
Comment 10 2010-12-29 09:39:42 PST
Alexey Proskuryakov
Comment 11 2010-12-29 13:08:06 PST
What does HTML5 say now?
Antti Koivisto
Comment 12 2010-12-29 13:30:38 PST
(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
Alexey Proskuryakov
Comment 13 2010-12-29 15:29:36 PST
> 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.
Darin Adler
Comment 14 2010-12-29 16:49:42 PST
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?
Darin Adler
Comment 15 2010-12-29 16:50:21 PST
(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.
Antti Koivisto
Comment 16 2010-12-29 16:50:42 PST
(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...
Antti Koivisto
Comment 17 2010-12-29 16:55:44 PST
> > 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)
Alexey Proskuryakov
Comment 18 2010-12-29 17:20:21 PST
> 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.
Antti Koivisto
Comment 19 2010-12-29 18:00:07 PST
(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!
Antti Koivisto
Comment 20 2010-12-30 13:24:37 PST
Alexey, I don't know how to address your concern. Perhaps you could suggest a way to move forward?
Alexey Proskuryakov
Comment 21 2010-12-30 13:34:14 PST
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.
Antti Koivisto
Comment 22 2010-12-30 14:30:34 PST
(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()).
Antti Koivisto
Comment 23 2010-12-30 14:34:06 PST
http://trac.webkit.org/changeset/33544 was the change that implemented most of this bug (making external script execution block on stylesheets)
Antti Koivisto
Comment 24 2010-12-30 14:35:40 PST
...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."
Alexey Proskuryakov
Comment 25 2010-12-30 14:36:00 PST
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.
Antti Koivisto
Comment 26 2010-12-30 15:04:52 PST
> 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?
Alexey Proskuryakov
Comment 27 2010-12-30 15:53:52 PST
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.
Antti Koivisto
Comment 28 2010-12-31 04:06:19 PST
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.
Antti Koivisto
Comment 29 2010-12-31 04:23:03 PST
Created attachment 77713 [details] modified the link element fix slightly to match the same test elsewhere (test for document changes too)
Dave Hyatt
Comment 30 2011-01-04 12:35:18 PST
Comment on attachment 77713 [details] modified the link element fix slightly to match the same test elsewhere (test for document changes too) r=me
Antti Koivisto
Comment 31 2011-01-04 12:46:50 PST
Kent Tamura
Comment 32 2011-01-04 15:46:06 PST
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.
Antti Koivisto
Comment 33 2011-01-05 03:56:34 PST
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
Kent Tamura
Comment 34 2011-01-05 16:15:20 PST
(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!
Tony Gentilcore
Comment 35 2011-03-17 14:57:31 PDT
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.
Tony Gentilcore
Comment 36 2011-03-22 10:41:51 PDT
(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.
Note You need to log in before you can comment on or make changes to this bug.