Bug 8760 – crash (hang?) on subtlegradient.com article page

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[tim bates]

I navigated to the above blog archive url (from an entry in my browsing history) 

Webkit crashed before the page had displayed any content (build  = 14216)

I don't see a crash, but it hangs intermittently, before anything is displayed, using the same build (r14216). I'm not sure what actions trigger it, but it hangs every time if I first load the page (sometimes no hang) and then refresh (hang every time). The same holds for the current release (2.0.3).

Setting priority P1, as I get a hang. And I guess a reduction would be nice.

Reporter, can you confirm that you had a crash and not a hang? I can't reproduce the crash. I think it would be useful if you could attach the crash reporter data.

[tim bates]

yes, it was a crash, not a hang. the crash log will have gone to Apple, but I didn't keep it for bugzilla, sorry.

The page caused Safari to loop infinitely in RenderStyle::getPseudoStyle with ps->pseudoStyle==ps. While trying to get down to the root cause I've discovered a few problems:
1. RenderStyle::getPseudoStyle will never break out of the while loop since it checks
            if (styleType() == pid)
instead of
            if (ps->styleType() == pid)
2. As a result of the above, RenderObject::getPseudoStyle will always call addPseudoStyle(), making for long redundant pseudo style lists.
3. In the special case of FIRST_LINE_INHERITED, since RenderObject::getPseudoStyle calls createStyleForElement(), it should set the styleType on the result before adding it.
4. Also in that case, if the style sheet hasn't loaded yet, createStyleForElement() will return the shared styleNotYetAvailable (even though the caller doesn't allow sharing). Combined with the 1-3 above, this will result in the styleNotYetAvailable shared style being added twice in a row to the same style, creating the cycle and thus the hang.

I started working on a fix, but fixing 1 appears to expose more serious problems which I still need to investigate.

Created attachment 8166 [details]
Patch, including change log but no layout test

I think in order to test this, you need a way to reproduce the "pending stylesheets" condition. Probably doable with the http server, but I haven't looked into it yet.

At first, I tried returning a non-shared copy of the styleNotYetAvailable when allowSharing was false, but this exposed another problem in case the element had "valid" style (presumably from updateLayoutIgnorePendingStylesheets()) but getPseudoStyle() was called after that (but before style sheets were loaded) and returned the non-shared styleNotYetAvailable for the first line of the element.

Comment on attachment 8166 [details]
Patch, including change log but no layout test

I cancelled the review request because this patch because  the fix to RenderStyle::getPseudoStyle() breaks fast/css-generated-content/hover-style-change.html. The reason for that is that you can't keep both :after and :hover:after in the pseudoStyle list. Since that part of the patch is not necessary for the fix, I think I'll take it out for now.

Created attachment 8169 [details]
Patch, including change log but no layout test

Please see my earlier comments. Took out the RenderStyle::getPseudoStyle() fix for now.

Created attachment 8170 [details]
Patch, including change log but no layout test

Corrected the change log. Please see my earlier comments.

Created attachment 8185 [details]
Patch, including change log and manual test

The test crashes TOT, but I couldn't get it to work as an automated test.

[Dave Hyatt]

Comment on attachment 8185 [details]
Patch, including change log and manual test

r=me.  Someone merging this patch should note that the getPseudoStyle FIXME is not needed if 8789 lands.

[Timothy Hatcher]

Landed in r14278