Bug 8760 - crash (hang?) on subtlegradient.com article page
Summary: crash (hang?) on subtlegradient.com article page
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P1 Critical
Assignee: Nobody
URL: http://subtlegradient.com/articles/20...
Keywords: NeedsReduction
Depends on:
Blocks:
 
Reported: 2006-05-06 14:20 PDT by tim bates
Modified: 2006-05-10 00:08 PDT (History)
1 user (show)

See Also:


Attachments
Patch, including change log but no layout test (3.78 KB, patch)
2006-05-08 10:10 PDT, mitz
no flags Details | Formatted Diff | Diff
Patch, including change log but no layout test (3.97 KB, patch)
2006-05-08 12:34 PDT, mitz
no flags Details | Formatted Diff | Diff
Patch, including change log but no layout test (3.87 KB, patch)
2006-05-08 12:43 PDT, mitz
no flags Details | Formatted Diff | Diff
Patch, including change log and manual test (4.66 KB, patch)
2006-05-09 10:13 PDT, mitz
hyatt: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description tim bates 2006-05-06 14:20:38 PDT
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)
Comment 1 jonathanjohnsson 2006-05-07 09:29:07 PDT
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.
Comment 2 tim bates 2006-05-07 10:40:50 PDT
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.
Comment 3 mitz 2006-05-07 15:15:43 PDT
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.
Comment 4 mitz 2006-05-08 10:10:46 PDT
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 5 mitz 2006-05-08 12:14:20 PDT
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.
Comment 6 mitz 2006-05-08 12:34:21 PDT
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.
Comment 7 mitz 2006-05-08 12:43:29 PDT
Created attachment 8170 [details]
Patch, including change log but no layout test

Corrected the change log. Please see my earlier comments.
Comment 8 mitz 2006-05-09 10:13:58 PDT
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.
Comment 9 Dave Hyatt 2006-05-09 14:23:26 PDT
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.
Comment 10 Timothy Hatcher 2006-05-10 00:08:55 PDT
Landed in r14278