|Summary:||RenderTableSection's setNeedsCellRecalc needs to null check table()|
|Product:||WebKit||Reporter:||James Robinson <jamesr>|
|Component:||New Bugs||Assignee:||James Robinson <jamesr>|
|Severity:||Normal||CC:||dglazkov, eric, hyatt, jamesr, mitz, simon.fraser|
|Version:||528+ (Nightly build)|
|OS:||OS X 10.5|
Description James Robinson 2011-01-19 17:11:30 PST
RenderTableSection's setNeedsCellRecalc needs to null check table()
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 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.