WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
Bug 18585
Frame::ownerRenderer() is likely causing strange crashes
https://bugs.webkit.org/show_bug.cgi?id=18585
Summary
Frame::ownerRenderer() is likely causing strange crashes
Eric Seidel (no email)
Reported
2008-04-18 10:08:03 PDT
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?
Attachments
cleanup patch which fixes layout issue and shows this crash!
(4.91 KB, patch)
2008-04-18 10:18 PDT
,
Eric Seidel (no email)
no flags
Details
Formatted Diff
Diff
Actually fix the ASSERT/potential crasher
(5.19 KB, patch)
2008-04-18 10:50 PDT
,
Eric Seidel (no email)
darin
: review+
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Eric Seidel (no email)
Comment 1
2008-04-18 10:18:56 PDT
Created
attachment 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
Eric Seidel (no email)
Comment 2
2008-04-18 10:32:52 PDT
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.
Eric Seidel (no email)
Comment 3
2008-04-18 10:33:26 PDT
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.
Eric Seidel (no email)
Comment 4
2008-04-18 10:44:38 PDT
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.
Eric Seidel (no email)
Comment 5
2008-04-18 10:50:06 PDT
Created
attachment 20671
[details]
Actually fix the ASSERT/potential crasher
Eric Seidel (no email)
Comment 6
2008-04-18 13:26:24 PDT
Hum.. I bet if some Apple person searched Radar or crashtracer there would be results related to this.
Darin Adler
Comment 7
2008-04-18 13:27:16 PDT
Comment on
attachment 20671
[details]
Actually fix the ASSERT/potential crasher + 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
Eric Seidel (no email)
Comment 8
2008-04-18 14:07:41 PDT
(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.
Eric Seidel (no email)
Comment 9
2008-04-18 15:03:32 PDT
Committed: e0d08dbe94fef979b381b6470fdb8a91e9be5c2f M WebCore/ChangeLog M WebCore/page/Frame.cpp M WebCore/rendering/RenderObject.h M WebCore/rendering/RenderPart.h Committed
r32232
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug