Bug 18585 - Frame::ownerRenderer() is likely causing strange crashes
: Frame::ownerRenderer() is likely causing strange crashes
Status: RESOLVED FIXED
: WebKit
Frames
: 528+ (Nightly build)
: Macintosh Mac OS X 10.5
: P1 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2008-04-18 10:08 PST by
Modified: 2008-04-18 15:03 PST (History)


Attachments
cleanup patch which fixes layout issue and shows this crash! (4.91 KB, patch)
2008-04-18 10:18 PST, Eric Seidel
no flags Review Patch | Details | Formatted Diff | Diff
Actually fix the ASSERT/potential crasher (5.19 KB, patch)
2008-04-18 10:50 PST, Eric Seidel
darin: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2008-04-18 10:08:03 PST
I was cleaning up Frame yesterday and added a few more ASSERTS.  One in ownerRenderer:

RenderPart* Frame::ownerRenderer() const
{
    HTMLFrameOwnerElement* ownerElement = d->m_ownerElement;
    if (!ownerElement)
        return 0;
    RenderObject* object = ownerElement->renderer();
    if (!object)
        return 0;
    ASSERT(object->isRenderPart());
    return static_cast<RenderPart*>(object);
}

This ASSERT fails with FOUR layout tests.  Yes, I was surprised too.  I expect that this could be causing all sorts of wacky crashes in real Safari.

I'm not sure why it's hitting these ASSERTs yet.  I'm also not sure if simply returning 0 is the right thing.  Why would a frame still think an element was owning it which wasn't?
------- Comment #1 From 2008-04-18 10:18:56 PST -------
Created an attachment (id=20670) [details]
cleanup patch which fixes layout issue and shows this crash!

I could mark this patch for review, since part of it fixes an issue where we would not be properly adjusting the view size when things change during relayout... I doubt it's seen very often.

But I won't yet since the ASSERT would cause 4 layout test failures.

This was based on the Frame cleanup patch I posted yesterday as part of bug 18556
------- Comment #2 From 2008-04-18 10:32:52 PST -------
The for cases which supposedly caused crashes:

Tests that caused the DumpRenderTree tool to crash:

fast/css/acid2                stderr
fast/table/giantRowspan2                stderr
http/tests/misc/acid2                stderr
http/tests/navigation/fallback-anchor-reload                stderr

I think fast/table/giantRowspan was bogus.  But http/tests/navigation/fallback-anchor-reload crashes reliably.
------- Comment #3 From 2008-04-18 10:33:26 PST -------
ASSERTION FAILED: object->isRenderPart()
(/Users/eseidel/Projects/WebKit/WebCore/page/Frame.cpp:1191 WebCore::RenderPart* WebCore::Frame::ownerRenderer() const)

Is the output, btw.  Exactly what was expected, the ASSERT is failing.
------- Comment #4 From 2008-04-18 10:44:38 PST -------
I think this is caused by <object> elements not disassociating themselves with the frames that they created when they decide to render fallback content (or any other content).  I imagine if you were to change an <object> from pointing at an .html file to pointing at a .png, it might crash in a similar manner.

It looks like there are only a few clients of ownerRenderer():

WebCore/page/FrameView.cpp:        RenderPart* renderer = m_frame->ownerRenderer();
WebCore/page/FrameView.cpp:        if (RenderPart* renderer = m_frame->ownerRenderer())
WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp:    if (frame->ownerRenderer())
WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp:        frame->ownerRenderer()->setWidget(frameView);
WebKit/mac/WebView/WebFrameView.mm:    if (RenderPart* owner = frame->ownerRenderer()) {
WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:    if (m_frame->ownerRenderer())
WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:        m_frame->ownerRenderer()->setWidget(frameView);
WebKit/win/WebCoreSupport/WebFrameLoaderClient.cpp:    if (frame->ownerRenderer())
WebKit/win/WebCoreSupport/WebFrameLoaderClient.cpp:        frame->ownerRenderer()->setWidget(frameView);

It looks safe to return 0 from ownerRenderer() so I'm going to fix this potential crash by doing so.  I'm not sure it's right for HTMLObjectElements to remain the ownerElement for these canceled/errored frames so long... but I guess we can deal with that later.
------- Comment #5 From 2008-04-18 10:50:06 PST -------
Created an attachment (id=20671) [details]
Actually fix the ASSERT/potential crasher
------- Comment #6 From 2008-04-18 13:26:24 PST -------
Hum.. I bet if some Apple person searched Radar or crashtracer there would be results related to this.
------- Comment #7 From 2008-04-18 13:27:16 PST -------
(From update of attachment 20671 [details])
+    if (!subtree && (!root->isRenderView() || !static_cast<RenderView*>(root)->printing()))

The isRenderView() check here isn't needed. If subtree is false, it's guaranteed to be a RenderView.

Change looks OK otherwise.

r=me either as-is or with that fix
------- Comment #8 From 2008-04-18 14:07:41 PST -------
(In reply to comment #7)
> (From update of attachment 20671 [details] [edit])
> +    if (!subtree && (!root->isRenderView() ||
> !static_cast<RenderView*>(root)->printing()))
> 
> The isRenderView() check here isn't needed. If subtree is false, it's
> guaranteed to be a RenderView.
> 
> Change looks OK otherwise.
> 
> r=me either as-is or with that fix
> 

I'll add a different ASSERT then.

Thx.
------- Comment #9 From 2008-04-18 15:03:32 PST -------
Committed: e0d08dbe94fef979b381b6470fdb8a91e9be5c2f
    M    WebCore/ChangeLog
    M    WebCore/page/Frame.cpp
    M    WebCore/rendering/RenderObject.h
    M    WebCore/rendering/RenderPart.h
Committed r32232