Downstream issue: http://code.google.com/p/chromium/issues/detail?id=101952 Chromium Win continuous builds (release only), experience strange behavior after r98542: 1. Start Chromium Release (Win) 2. Hit Ctrl + J (brings up downloads page) Expected: all fine Actual: CSS text is rendered in the beginning of the page. Regressed in: http://trac.webkit.org/changeset/98542
Note that accessing document properties in the JavaScript or opening inspector fixes the rendering.
I can't debug Chromium Win UI features. You should come up with an HTML test case. That should not be too hard assuming Chromium is doing sensible things (and if not then it might be a Chromium bug).
There isn't really much information on this bug. Do we have any idea how this affects other ports? We believe this is only a Chromium regression? We're certain it started with r98542 (seems plausible)?
I'm confused. http://code.google.com/p/chromium/issues/detail?id=101952 Talks about the inspector not reloading when a tab crashes. Yet the comments in this bug talk about CSS text being rendered at the begining of a page? Can we have a screenshot? What's the inspector doing in Chromium that the Inspector isn't doing in Safari? Does this affect the Safari inspector?
(In reply to comment #4) > I'm confused. http://code.google.com/p/chromium/issues/detail?id=101952 Talks about the inspector not reloading when a tab crashes. Yet the comments in this bug talk about CSS text being rendered at the begining of a page? > > Can we have a screenshot? What's the inspector doing in Chromium that the Inspector isn't doing in Safari? Does this affect the Safari inspector? That's because I posted the wrong downstream bug, sorry. Downstream bug: http://code.google.com/p/chromium/issues/detail?id=102046 One of the dupes containing screenshot: http://code.google.com/p/chromium/issues/detail?id=102352
(In reply to comment #3) > There isn't really much information on this bug. Do we have any idea how this affects other ports? We believe this is only a Chromium regression? We're certain it started with r98542 (seems plausible)? I was rolling out CSS-related patches from the roll one by one: rolled out http://trac.webkit.org/changeset/98568 - still repros rolled out http://trac.webkit.org/changeset/98573 - still repros rolled out http://trac.webkit.org/changeset/98542 - problem vanished.
(In reply to comment #6) > (In reply to comment #3) > > There isn't really much information on this bug. Do we have any idea how this affects other ports? We believe this is only a Chromium regression? We're certain it started with r98542 (seems plausible)? > > I was rolling out CSS-related patches from the roll one by one: > rolled out http://trac.webkit.org/changeset/98568 - still repros > rolled out http://trac.webkit.org/changeset/98573 - still repros > rolled out http://trac.webkit.org/changeset/98542 - problem vanished. OK. Just so I understand, I'll restate: You took a tip-of-tree checkout and reverted exactly r98542 and the problem went away? Or you rolled back to r98541? Just curious. It seems odd that CSS text would get rendered. Unless the behavior of the HTMLStyleElement changed? (I haven't read Antti's change in great detail yet.)
Do you believe this bug requires use of the "experimental extensions API" mentioned in http://code.google.com/p/chromium/issues/detail?id=102046 to trigger? Or could this be triggered in other scenarios?
> > I was rolling out CSS-related patches from the roll one by one: > > rolled out http://trac.webkit.org/changeset/98568 - still repros > > rolled out http://trac.webkit.org/changeset/98573 - still repros > > rolled out http://trac.webkit.org/changeset/98542 - problem vanished. > > OK. Just so I understand, I'll restate: You took a tip-of-tree checkout and reverted exactly r98542 and the problem went away? Or you rolled back to r98541? Just curious. > I took: Chromium r107577 (http://src.chromium.org/viewvc/chrome?view=rev&revision=107577) and WebKit r98582. I.e. exact post-roll state. Then I rolled out the ones above one-by-one. The problem went away once I rolled out 98542.
(In reply to comment #8) > Do you believe this bug requires use of the "experimental extensions API" mentioned in http://code.google.com/p/chromium/issues/detail?id=102046 to trigger? Or could this be triggered in other scenarios? I believe it was a clean profile on the windows box. It only repros on Release, I could not repro on Debug. Also, once you touch DOM programmatically (via JS), layout was getting in place.
Wow. It appears this has had at least *25* separate bug reports filed on this regression in crbugs.com in the last week? At least thats what it seems from following the dupe links in the bug you cited in comment 6.
I'm confused why any css should show up as text? That would require something giving a <style> element a renderer and laying it out like a span/div? Some of the reports claim this is related to chrome:// urls: http://code.google.com/p/chromium/issues/detail?id=102352 What is the downloads page doing to "inject" that CSS? Or is the downloads page some (mostly) static HTML? How is it loaded into WebKit?
It seems all of the duplicates are about chrome://downloads? What is chrome://downloads doing that is special?
(In reply to comment #13) > It seems all of the duplicates are about chrome://downloads? What is chrome://downloads doing that is special? It looks static to me. I did not think of saving the downloads page and opening it from file:// while I had access to the win box.
Is my understanding correct that this has only occurred on Chromium, only on Windows, and only on Release?
http://code.google.com/p/chromium/issues/detail?id=102648 contains a copy of the CSS seen on the downloads page as text.
(In reply to comment #16) > http://code.google.com/p/chromium/issues/detail?id=102648 contains a copy of the CSS seen on the downloads page as text. So I tried downloading the chrome://downloads and loading it off file:// and off localhost:8000. In both cases, I did not observe the issue.
(In reply to comment #17) > (In reply to comment #16) > > http://code.google.com/p/chromium/issues/detail?id=102648 contains a copy of the CSS seen on the downloads page as text. > > So I tried downloading the chrome://downloads and loading it off file:// and off localhost:8000. In both cases, I did not observe the issue. I've not seen a report in that tree of bugs for any page other than chrome://downloads (or the one for the experimental insert css api). Which on the surface suggests of a chrome-using-webkit-strangely bug, rather than a WebKit-doing-something-wrong bug. We need more information about the reproduction before there is much to do from WebKit's side. We could of course roll out the patch, but without better understanding, it would be impossible for Antti to ever roll it back in (since he doesn't work on Chrome). :(
http://code.google.com/p/chromium/issues/detail?id=102521 Google maps fails to render on the latest build I've traced this bug to this patch too.
(In reply to comment #19) > http://code.google.com/p/chromium/issues/detail?id=102521 Google maps fails to render on the latest build > I've traced this bug to this patch too. Is that also a Chromium-specific bug or does it affect other WebKit browsers too?
The maps.google.com direction icon rendering problem affects all WebKit browsers (at least, I've tested Chrome Canary and WebKit Nightly on MacOS 10.6). David has filed a bug for this https://bugs.webkit.org/show_bug.cgi?id=71996.
I was able to reduce this down to two small static files on Chrome Canary 17.0.935.0 for Win7. I'll try to repro with a webkit nightly. Any of the following changes make the problem go away: - Removing the <!DOCTYPE HTML> - Inlining the local_strings.js script - Moving the <script src> out of the head and into the body. - Having a body which doesn't include an anchor element - Removing the script which changes the <html>'s element dir attribute. downloads.html -------------- <!DOCTYPE HTML> <html> <head> <style> .header { overflow: auto; clear: both; } </style> <script src="local_strings.js"> </script> </head> <body> <a href=""></a> <script> document.childNodes[1].setAttribute("dir", "ltr"); </script> </body> </html> local_strings.js ----------------- function Foo() { }
Did I understand correctly that the CSS text should show up on the page when loading this test file? I don't see that happening on TOT WebKit Safari/Mac build. Could you attach the test files to remove the possibility of copy-paste errors? Looking at the test it seems unlikely that this is a dupe of Bug 71996 (which was fixed) though it might be something similar.
Created attachment 114857 [details] downloads.html
Created attachment 114858 [details] local_strings.js
Comment on attachment 114857 [details] downloads.html Two static files that reproduce the problem on Win7 Chrome 17.0.937.0 The text in the CSS file is visible in the document when viewed. Reloads make it go away.
Here are the two files which reproduce the problem on Chrome Win7 17.0.937.0 The text inside the <style> section is visible on the page. Reloads make it go away.
From the chromium bugtracker, by Satoshi.Matsuzaki: The issue on chrome://downloads and chrome://history2 was resolved on Linux, after WebKit changeset 99873. Confirmed with local custom DEPS builds. Chromum 109521 + WebKit 99872: FAIL Chromum 109521 + WebKit 99873: OK But the issue is still happening on Windows version, even on ToT (109826).
(In reply to comment #28) > From the chromium bugtracker, by Satoshi.Matsuzaki: > The issue on chrome://downloads and chrome://history2 was resolved on Linux, > after WebKit changeset 99873. Confirmed with local custom DEPS builds. > Chromum 109521 + WebKit 99872: FAIL > Chromum 109521 + WebKit 99873: OK > But the issue is still happening on Windows version, even on ToT (109826). That sounds like a possible build dependency issue. :)
Since so far only Chrome was tested here, i tried TOT WebKit with Windows Safari: The CSS code was visible at the first time i opened the file (but in further attempts it wasn't, at least not without reseting the cache...but it seems the r99984 build i downloaded is really not stable in general)
I can't repro (I only have OSX dev environment) and can't find any obvious reason with code inspection. Very likely it is something to do with explicitly setting the dir attribute. That involves some messy code. If you can repro, it should be relatively easy to debug. The likely way the content of <style> would become visible is that the display attribute value for the element (RenderStyle computed by CSSStyleSelector::styleForElement) gets miscalculated or clobbered somehow. Since the test case is small you can pretty much step through the entire process to see where it goes wrong.
I mean "display property". The default style sheet has "style { display: none }" so that needs to get overriden somehow.
Unable to repro on Win7 WebKit nightly r100095 (built 14 November 2011). This may be a Chromium specific issue.
(In reply to comment #33) > Unable to repro on Win7 WebKit nightly r100095 (built 14 November 2011). This may be a Chromium specific issue. i did repro it in Safari (comment #30), will try a newer build of WebKit later today...
weird: i tried again with Windows Safari using WebKit r100145 1. On first attempt the CSS wasn't visible 2. After refreshing the page "Windows Problem Reporting" (werfault.exe) asked my firewall to connect the network (something crashed behind the scenes) and the CSS code showed up in the refreshed page. it seems WebKit2WebProcess.exe keeps crashing when i try to surf using the latest WebKit builds on Windows (not just in this testcase)
Created attachment 115731 [details] Patch
Comment on attachment 115731 [details] Patch How do we test this?
Actually, I just verified that this can be reproduced with DumpRenderTree.exe with the attached test case minus the external script.
I get the following with Chromium Windows DumpRenderTree without the patch: layer at (0,0) size 800x600 RenderView at (0,0) size 800x600 layer at (0,0) size 800x28 RenderBlock {HTML} at (0,0) size 800x28 RenderBlock (anonymous) at (0,0) size 800x0 RenderInline {HEAD} at (0,0) size 0x0 RenderText {#text} at (0,0) size 0x0 RenderBlock (anonymous) at (0,0) size 800x20 RenderBlock {STYLE} at (0,0) size 800x20 RenderText {#text} at (0,0) size 233x19 text run at (0,0) width 60: ".header { " text run at (60,0) width 94: "overflow: auto; " text run at (154,0) width 71: "clear: both; " text run at (225,0) width 8: "}" RenderBlock (anonymous) at (0,20) size 800x0 RenderInline {HEAD} at (0,0) size 0x0 RenderText {#text} at (0,0) size 0x0 RenderBody {BODY} at (8,28) size 784x0 RenderInline {A} at (0,0) size 0x0 [color=#551A8B] RenderText {#text} at (0,0) size 0x0 RenderText {#text} at (0,0) size 0x0 With the patch I get (also without it on other platforms): layer at (0,0) size 800x600 RenderView at (0,0) size 800x600 layer at (0,0) size 800x8 RenderBlock {HTML} at (0,0) size 800x8 RenderBody {BODY} at (8,8) size 784x0 RenderInline {A} at (0,0) size 0x0 [color=#551A8B] RenderText {#text} at (0,0) size 0x0 RenderText {#text} at (0,0) size 0x0
Created attachment 115738 [details] Patch
Comment on attachment 115738 [details] Patch Can we make this test more clear? Maybe by use of getElementById('target') or whatever node childNodes[1] is addressing?
Thanks Noel for talking through the bug with me. It really helped to cement my understanding of the problem.
(In reply to comment #41) > (From update of attachment 115738 [details]) > Can we make this test more clear? Maybe by use of getElementById('target') or whatever node childNodes[1] is addressing? Yes, we could say documentElement instead of childNodes[1].
Comment on attachment 115738 [details] Patch I'll wait for the review comments to accumulate before resubmitting the patch.
Comment on attachment 115738 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=115738&action=review > Source/WebCore/css/CSSStyleSelector.cpp:1268 > ensureDefaultStyleSheetsForElement(element); I think it would be clearer to either: 1) Make ensureDefualtStyleSheetsForElement return a boolean indicating that the cache needs to be invalidated because the default style has changed, or 2) to use a setter that automatically clears the cache when called. As it is this code is essentially watching to see if a side-effect has occurred, which makes the intent less clear than it could be. Another solution might be something like: if (simpleDefaultStyleSheet && !elementCanUseSimpleDefaultStyle(element)) return s_styleNotYetAvailable; (Not 100% sure on that, but Antti should know.)
Comment on attachment 115738 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=115738&action=review > Source/WebCore/css/CSSStyleSelector.cpp:1270 > + if (wasSimpleDefaultStyleSheet && defaultStyle) > + m_matchStyleDeclarationCache.clear(); This code change won’t completely fix the problem. That’s because when loadFullDefaultStyle is called, *all* CSSStyleSelector objects would need to have their match style declarations caches cleared. Not just the one that triggered the call to loadFullDefaultStyle. Not sure the best way to structure the code to make that happen, though.
Great work on the diagnosis, by the way!
Thanks for debugging, I should have figured it out from the symptoms. I think we should just disable the cache until we have full default stylesheet loaded. It is not really useful with very simple style anyway.
(In reply to comment #48) > Thanks for debugging, I should have figured it out from the symptoms. I think we should just disable the cache until we have full default stylesheet loaded. It is not really useful with very simple style anyway. Thanks for the feedback, the alternate approach is much cleaner.
Created attachment 115905 [details] Patch
Comment on attachment 115905 [details] Patch Just tested the alternate patch with Chrome Win and it fails the test in the browser although DumpRenderTree passes. :'(
That won't work, the correct test is !simpleDefaultStyleSheet It would be better to treat it as non-cacheable already here (setting hash to zero) so you don't even try a lookup. 2283void CSSStyleSelector::applyMatchedDeclarations(const MatchResult& matchResult) 2284{ 2285 unsigned cacheHash = matchResult.isCacheable ? computeDeclarationHash(m_matchedDecls.data(), m_matchedDecls.size()) : 0;
Created attachment 115932 [details] Patch
(In reply to comment #52) > That won't work, the correct test is !simpleDefaultStyleSheet > > It would be better to treat it as non-cacheable already here (setting hash to zero) so you don't even try a lookup. Thank you Antti, your comments were spot on. I just uploaded a new patch that actually passes. :)
Comment on attachment 115932 [details] Patch I think Antti and I are in agreement on the solution now.
Created attachment 115934 [details] Patch
Comment on attachment 115934 [details] Patch Sorry for the noise, I neglected to address a few nits in the last patch.
Comment on attachment 115934 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=115934&action=review r=me, please keep an eye on that the test actually passes on all platforms when landing. > Source/WebCore/css/CSSStyleSelector.cpp:2285 > void CSSStyleSelector::applyMatchedDeclarations(const MatchResult& matchResult) > { > - unsigned cacheHash = matchResult.isCacheable ? computeDeclarationHash(m_matchedDecls.data(), m_matchedDecls.size()) : 0; > + unsigned cacheHash = !simpleDefaultStyleSheet && matchResult.isCacheable ? computeDeclarationHash(m_matchedDecls.data(), m_matchedDecls.size()) : 0; The ternary statement is getting bit ugly. You could just set isCacheable to false beforehand, that reads better too. > LayoutTests/fast/css/style-tag-display-none-expected.txt:8 > +layer at (0,0) size 800x600 > + RenderView at (0,0) size 800x600 > +layer at (0,0) size 800x8 > + RenderBlock {HTML} at (0,0) size 800x8 > + RenderBody {BODY} at (8,8) size 784x0 > + RenderInline {A} at (0,0) size 0x0 [color=#551A8B] > + RenderText {#text} at (0,0) size 0x0 > + RenderText {#text} at (0,0) size 0x0 Not sure if this render tree dump is actually valid on all platforms.
(In reply to comment #58) >The ternary statement is getting bit ugly. You could just set isCacheable to false beforehand, that reads better too. The most stylish place to do this is in CSSStyleSelector::matchUARules.
Created attachment 115947 [details] Patch
Comment on attachment 115947 [details] Patch I've confirmed that this patch resolves the issue with Chromium Win. Antti, thanks again for the feedback, correctness and elegance need not be mutually exclusive.
Comment on attachment 115947 [details] Patch Clearing flags on attachment: 115947 Committed r100856: <http://trac.webkit.org/changeset/100856>
All reviewed patches have been landed. Closing bug.
Rolled out in http://trac.webkit.org/changeset/100896
(In reply to comment #64) > Rolled out in http://trac.webkit.org/changeset/100896 But why? What did it break and how? Are we sure the test didn't just need a rebaseline?
Created attachment 116160 [details] Patch
Comment on attachment 116160 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=116160&action=review > LayoutTests/ChangeLog:10 > + * platform/chromium/fast/css/style-tag-display-none-expected.png: Added. This should be in fast/css, since it'll be the same result for all platforms.
Created attachment 116165 [details] Patch
Comment on attachment 116165 [details] Patch Clearing flags on attachment: 116165 Committed r100976: <http://trac.webkit.org/changeset/100976>