Bug 17902

Summary: Acid3 sometimes crashes in WebCore::RenderObject::setNeedsLayout
Product: WebKit Reporter: Robert Blaut <webkit>
Component: Page LoadingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Major CC: ismail, koivisto, mitz, zimmermann
Priority: P1 Keywords: InRadar, NeedsReduction
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: OS X 10.5   
URL: http://acid3.acidtests.org
Attachments:
Description Flags
crash log
none
Speculative fix, based on backtrace oliver: review+

Robert Blaut
Reported 2008-03-17 15:30:13 PDT
In Webkit r31112 on Mac OS X I experience now crash in WebCore::RenderObject::setNeedsLayout. It's hard to reproduce but it's reproducible. All you should do is reloading and reloading the test. Often stop in the middle of running of test and reload. It's a sort of torture test but always leads to the same crash. It's related to the bug 17878 which is already fixed.
Attachments
crash log (24.68 KB, text/plain)
2008-03-17 15:32 PDT, Robert Blaut 		no flags
Details
Speculative fix, based on backtrace (752 bytes, patch)
2008-03-22 23:40 PDT, Eric Seidel (no email) 		oliver: review+
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Robert Blaut
Comment 1 2008-03-17 15:32:45 PDT
Created attachment 19853 [details] crash log
Mark Rowe (bdash)
Comment 2 2008-03-17 15:48:07 PDT
I was able to reproduce this in the latest nightly build (r31090) after refreshing many times, closing the window, then loading Acid3 once more.
Mark Rowe (bdash)
Comment 3 2008-03-17 15:48:25 PDT
<rdar://problem/5803814>
Antti Koivisto
Comment 4 2008-03-17 21:06:54 PDT
Are you sure this is a regression? I suspect this might be unrelated to the other loader crashes. Based on the crash log renderer() just should be null checked here: void CSSFontSelector::fontLoaded(CSSSegmentedFontFace*) { if (m_document->inPageCache()) return; m_document->recalcStyle(Document::Force); m_document->renderer()->setNeedsLayoutAndPrefWidthsRecalc(); }
Antti Koivisto
Comment 5 2008-03-18 16:05:01 PDT
Removing regression keyword since there is no evidence this is one.
mitz
Comment 6 2008-03-18 16:11:20 PDT
(In reply to comment #4) > Are you sure this is a regression? I suspect this might be unrelated to the > other loader crashes. Based on the crash log renderer() just should be null > checked here: Why are we reaching this code with a viewless document? Is it in the page cache or is it the document holding the remote font (in which case we should probably be targeting a different document, the one using the font)?
Antti Koivisto
Comment 7 2008-03-18 17:05:20 PDT
I can't reproduce so I only have the crash log to look. CSSFontSelector must have reffed the CachedFont since it is getting the callback. Cache layer is not making any decisions which document to target, it is all subscription based. Architecturally it is wrong to assume that all documents have render tree. Are there no real scenarios you can think of where we would either have ripped down the render tree or not yet constructed it?
mitz
Comment 8 2008-03-18 17:20:39 PDT
I can't repro either (and I don't think that the bug has to do with your recent loader work). If a document does not have a renderer it should not be asking for fonts. If it had asked for them while it had a renderer and then lost it, it should have cancelled the request.
Antti Koivisto
Comment 9 2008-03-18 18:05:52 PDT
I don't see that that reflected in code. Fonts are requested in CSS parsing time and are in no way tied to rendering as far as I see.
Eric Seidel (no email)
Comment 10 2008-03-22 23:40:52 PDT
Created attachment 19978 [details] Speculative fix, based on backtrace WebCore/css/CSSFontSelector.cpp | 6 ++++-- 1 files changed, 4 insertions(+), 2 deletions(-)
Eric Seidel (no email)
Comment 11 2008-03-22 23:41:25 PDT
Comment on attachment 19978 [details] Speculative fix, based on backtrace Suggestions as to how I might make a test for this (i.e. when the document would not have a renderer, yet would be loading fonts) would be most welcome!
Ismail Donmez
Comment 12 2008-03-22 23:49:48 PDT
To reproduce refresh acid3 test 10 times and each time wait for it to finish. Then close Safari and open acid3 test again, it'll crash.
Eric Seidel (no email)
Comment 13 2008-03-25 13:07:02 PDT
*** Bug 17672 has been marked as a duplicate of this bug. ***
Oliver Hunt
Comment 14 2008-03-25 14:25:54 PDT
Comment on attachment 19978 [details] Speculative fix, based on backtrace Changelog, and testcase needed r=me
Eric Seidel (no email)
Comment 15 2008-03-25 14:31:11 PDT
landed as r31290. Sadly, w/o test case. :(
Note You need to log in before you can comment on or make changes to this bug.