Bug 17902 - Acid3 sometimes crashes in WebCore::RenderObject::setNeedsLayout
Summary: Acid3 sometimes crashes in WebCore::RenderObject::setNeedsLayout
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P1 Major
Assignee: Nobody
URL: http://acid3.acidtests.org
Keywords: InRadar, NeedsReduction
Depends on:
Blocks:
 
Reported: 2008-03-17 15:30 PDT by Robert Blaut
Modified: 2008-03-25 14:31 PDT (History)
4 users (show)

See Also:


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+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Robert Blaut 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.
Comment 1 Robert Blaut 2008-03-17 15:32:45 PDT
Created attachment 19853 [details]
crash log
Comment 2 Mark Rowe (bdash) 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.
Comment 3 Mark Rowe (bdash) 2008-03-17 15:48:25 PDT
<rdar://problem/5803814>
Comment 4 Antti Koivisto 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();
}
Comment 5 Antti Koivisto 2008-03-18 16:05:01 PDT
Removing regression keyword since there is no evidence this is one.
Comment 6 mitz 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)?
Comment 7 Antti Koivisto 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?
Comment 8 mitz 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.
Comment 9 Antti Koivisto 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.
Comment 10 Eric Seidel (no email) 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(-)
Comment 11 Eric Seidel (no email) 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!
Comment 12 Ismail Donmez 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.
Comment 13 Eric Seidel (no email) 2008-03-25 13:07:02 PDT
*** Bug 17672 has been marked as a duplicate of this bug. ***
Comment 14 Oliver Hunt 2008-03-25 14:25:54 PDT
Comment on attachment 19978 [details]
Speculative fix, based on backtrace

Changelog, and testcase needed 
r=me
Comment 15 Eric Seidel (no email) 2008-03-25 14:31:11 PDT
landed as r31290.  Sadly, w/o test case. :(