Bug 71703 – REGRESSION(r98542): Chromium: CSS text is rendered on page

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Pavel Feldman]

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

[Pavel Feldman]

Note that accessing document properties in the JavaScript or opening inspector fixes the rendering.

[Antti Koivisto]

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

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

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?

[Pavel Feldman]

(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

[Pavel Feldman]

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

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

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?

[Pavel Feldman]

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

[Pavel Feldman]

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

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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?

[Eric Seidel (no email)]

It seems all of the duplicates are about chrome://downloads?  What is chrome://downloads doing that is special?

[Pavel Feldman]

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

[Eric Seidel (no email)]

Is my understanding correct that this has only occurred on Chromium, only on Windows, and only on Release?

[Eric Seidel (no email)]

http://code.google.com/p/chromium/issues/detail?id=102648 contains a copy of the CSS seen on the downloads page as text.

[Pavel Feldman]

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

[Eric Seidel (no email)]

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

[David Barr]

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.

[Darin Adler]

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

[Mike Lawther]

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() {
}

[Antti Koivisto]

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.

[Yair Yogev]

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

[Eric Seidel (no email)]

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

[Yair Yogev]

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)

[Antti Koivisto]

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.

[Antti Koivisto]

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.

[Yair Yogev]

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

[Yair Yogev]

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)

[David Barr]

Created attachment 115731 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 115731 [details]
Patch

How do we test this?

[David Barr]

Actually, I just verified that this can be reproduced with DumpRenderTree.exe with the attached test case minus the external script.

[David Barr]

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

[David Barr]

Created attachment 115738 [details]
Patch

[Eric Seidel (no email)]

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?

[David Barr]

Thanks Noel for talking through the bug with me. It really helped to cement my understanding of the problem.

[David Barr]

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

[David Barr]

Comment on attachment 115738 [details]
Patch

I'll wait for the review comments to accumulate before resubmitting the patch.

[Luke Macpherson]

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

[Darin Adler]

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.

[Darin Adler]

Great work on the diagnosis, by the way!

[Antti Koivisto]

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.

[David Barr]

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

[David Barr]

Created attachment 115905 [details]
Patch

[David Barr]

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

[Antti Koivisto]

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;

[David Barr]

Created attachment 115932 [details]
Patch

[David Barr]

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

[David Barr]

Comment on attachment 115932 [details]
Patch

I think Antti and I are in agreement on the solution now.

[David Barr]

Created attachment 115934 [details]
Patch

[David Barr]

Comment on attachment 115934 [details]
Patch

Sorry for the noise, I neglected to address a few nits in the last patch.

[Antti Koivisto]

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.

[Antti Koivisto]

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

[David Barr]

Created attachment 115947 [details]
Patch

[David Barr]

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.

[WebKit Review Bot]

Comment on attachment 115947 [details]
Patch

Clearing flags on attachment: 115947

Committed r100856: <http://trac.webkit.org/changeset/100856>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Steve Block]

Rolled out in http://trac.webkit.org/changeset/100896

[Eric Seidel (no email)]

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

[David Barr]

Created attachment 116160 [details]
Patch

[Dimitri Glazkov (Google)]

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.

[David Barr]

Created attachment 116165 [details]
Patch

[WebKit Review Bot]

Comment on attachment 116165 [details]
Patch

Clearing flags on attachment: 116165

Committed r100976: <http://trac.webkit.org/changeset/100976>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.