Bug 52770 - RenderTableSection's setNeedsCellRecalc needs to null check table()
Summary: RenderTableSection's setNeedsCellRecalc needs to null check table()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: James Robinson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-01-19 17:11 PST by James Robinson
Modified: 2011-01-20 13:34 PST (History)
6 users (show)

See Also:


Attachments
Patch (1.96 KB, patch)
2011-01-19 17:12 PST, James Robinson
no flags Details | Formatted Diff | Diff
css from the extension (this is injected into every page) (5.86 KB, text/plain)
2011-01-19 18:22 PST, Eric Seidel (no email)
no flags Details
stand-alone reduction (crashes when you reload the page, no extensions required!) (92 bytes, text/html)
2011-01-19 19:05 PST, Eric Seidel (no email)
no flags Details
layout test (152 bytes, text/plain)
2011-01-19 19:10 PST, Eric Seidel (no email)
no flags Details
Patch (3.94 KB, patch)
2011-01-19 19:48 PST, James Robinson
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description James Robinson 2011-01-19 17:11:30 PST
RenderTableSection's setNeedsCellRecalc needs to null check table()
Comment 1 James Robinson 2011-01-19 17:12:20 PST
Created attachment 79526 [details]
Patch
Comment 2 James Robinson 2011-01-19 17:17:52 PST
This patch fixes the crash reported in http://code.google.com/p/chromium/issues/detail?id=69212.  It's obvious from inspector that this should check for NULL but I'm not sure exactly how to make a layout tests with a RenderTableSection that has a null parent.  The repro instructions on the chromium bug (install https://chrome.google.com/webstore/detail/hngfmkbjhlcbdgmppkpkdejbgmblalmi, go to http://dicademusica.blogspot.com/2011/01/fotos-engracadas-com-mulheres.html, reload) worked for me.  I'm guessing the failure is related to one of the ::-webkit-scrollbar styles that https://chrome.google.com/webstore/detail/hngfmkbjhlcbdgmppkpkdejbgmblalmi sets.
Comment 3 Eric Seidel (no email) 2011-01-19 18:22:13 PST
Created attachment 79534 [details]
css from the extension (this is injected into every page)

Should be possible to copy/paste this CSS at the top of a local version of the page in question to reproduce the crash, no?

The extension also has a background.html page, but that doesn't seem related.
Comment 4 Eric Seidel (no email) 2011-01-19 18:30:38 PST
The naive approach to a test case didn't work, sadly.

I might try reducing the extension's CSS instead.
Comment 5 James Robinson 2011-01-19 18:35:29 PST
The page doesn't seem to crash immediately, so it seems that there's some interaction with some delayed resource that leads to the crash.  It may take some fiddling to get it just right (sadly).
Comment 6 Eric Seidel (no email) 2011-01-19 18:37:51 PST
Using a local copy of the extension (which I"m about to reduce) I'm able to crash with the live site.  I'm not able to crash with a curl'd down copy of the site however.  (Yes, my local extension is loaded for file urls.)
Comment 7 Eric Seidel (no email) 2011-01-19 18:57:40 PST
<iframe src="http://static.megacubo.net/live/banners/top468.html?c=ffffff,000000" width="468" height="60" frameborder="0"></iframe>

Is the code causing the crash.
Comment 8 Eric Seidel (no email) 2011-01-19 19:05:54 PST
Created attachment 79539 [details]
stand-alone reduction (crashes when you reload the page, no extensions required!)
Comment 9 Eric Seidel (no email) 2011-01-19 19:10:48 PST
Created attachment 79540 [details]
layout test
Comment 10 James Robinson 2011-01-19 19:48:44 PST
Created attachment 79542 [details]
Patch
Comment 11 James Robinson 2011-01-19 19:49:17 PST
Mad props to Eric for the reduction.  How's this?
Comment 12 Eric Seidel (no email) 2011-01-19 20:46:14 PST
Comment on attachment 79542 [details]
Patch

No shout-out in the ChangeLog?  How will the interwebs know how baller I was here w/o it? :)
Comment 13 Eric Seidel (no email) 2011-01-19 20:46:58 PST
Comment on attachment 79542 [details]
Patch

Are there perf concerns with moving this function to be non-inline?  I kinda doubt it, but curious. I guess we'll see when the PLT bots run.
Comment 14 James Robinson 2011-01-20 13:31:39 PST
(In reply to comment #13)
> (From update of attachment 79542 [details])
> Are there perf concerns with moving this function to be non-inline?  I kinda doubt it, but curious. I guess we'll see when the PLT bots run.

I don't think this function is super hot, but it's getting a little big to be inline at this point.
Comment 15 James Robinson 2011-01-20 13:34:55 PST
Committed r76276: <http://trac.webkit.org/changeset/76276>