Summary: | Frame::ownerRenderer() is likely causing strange crashes | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Eric Seidel (no email) <eric> | ||||||
Component: | Frames | Assignee: | Eric Seidel (no email) <eric> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | ||||||||
Priority: | P1 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Mac | ||||||||
OS: | OS X 10.5 | ||||||||
Attachments: |
|
Description
Eric Seidel (no email)
2008-04-18 10:08:03 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 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. 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. 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. Created attachment 20671 [details]
Actually fix the ASSERT/potential crasher
Hum.. I bet if some Apple person searched Radar or crashtracer there would be results related to this. 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
(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. |