344291 - [FIX]Crash [@ nsIView::Destroy] on 1.8.0.5 and 1.8.1 branch

Status: VERIFIED FIXED

(Core :: Layout, defect, P1)

Description

[Martijn Wargers (dead)]

See upcoming testcase which crashes 1.8.0.5 branch and 1.8.1 branch, it doesn't crash current trunk build.
If desired, I can reduce the testcase, which I'll atttach shortly.

Talkback ID: TB20839728G
0x00000000
nsIView::Destroy  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/view/src/nsView.cpp, line 305]
nsSplittableFrame::Destroy  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/layout/generic/nsSplittableFrame.cpp, line 71]
nsBlockFrame::Destroy  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/layout/generic/nsBlockFrame.cpp, line 317]
nsLineBox::DeleteLineList  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/layout/generic/nsLineBox.cpp, line 325]
nsLineBox::DeleteLineList  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/layout/generic/nsLineBox.cpp, line 325]
nsLineBox::DeleteLineList  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/layout/generic/nsLineBox.cpp, line 325]
nsLineBox::DeleteLineList  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/layout/generic/nsLineBox.cpp, line 325]
nsLineBox::DeleteLineList  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/layout/generic/nsLineBox.cpp, line 325]
nsLineBox::DeleteLineList  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/layout/generic/nsLineBox.cpp, line 325]
nsLineBox::DeleteLineList  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/layout/generic/nsLineBox.cpp, line 325]
nsBlockFrame::DoRemoveFrame  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/layout/generic/nsBlockFrame.cpp, line 5718]
nsBlockFrame::RemoveFrame  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/layout/generic/nsBlockFrame.cpp, line 5510]
nsFrameManager::RemoveFrame  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/layout/base/nsFrameManager.cpp, line 705]
nsCSSFrameConstructor::ContentRemoved  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/layout/base/nsCSSFrameConstructor.cpp, line 10071]
nsCSSFrameConstructor::ReinsertContent  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/layout/base/nsCSSFrameConstructor.cpp, line 9611]
nsCSSFrameConstructor::MaybeRecreateContainerForIBSplitterFrame  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/layout/base/nsCSSFrameConstructor.cpp, line 11891]
nsCSSFrameConstructor::RestyleElement  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/layout/base/nsCSSFrameConstructor.cpp, line 10503]
nsCSSFrameConstructor::ProcessOneRestyle  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/layout/base/nsCSSFrameConstructor.cpp, line 13944]
nsCSSFrameConstructor::ProcessPendingRestyles  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/layout/base/nsCSSFrameConstructor.cpp, line 13992]
PresShell::FlushPendingNotifications  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/layout/base/nsPresShell.cpp, line 5338]
nsDocument::FlushPendingNotifications  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/content/base/src/nsDocument.cpp, line 4272]
nsHTMLDocument::FlushPendingNotifications  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/content/html/document/src/nsHTMLDocument.cpp, line 1271]
nsHTMLExternalObjSH::GetPluginInstance  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/dom/src/base/nsDOMClassInfo.cpp, line 8768]
nsHTMLExternalObjSH::PostCreate  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/dom/src/base/nsDOMClassInfo.cpp, line 8828]
XPCWrappedNative::GetNewOrUsed  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/js/src/xpconnect/src/xpcwrappednative.cpp, line 467]
XPCConvert::NativeInterface2JSObject  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/js/src/xpconnect/src/xpcconvert.cpp, line 1156]
nsXPConnect::WrapNative  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/js/src/xpconnect/src/nsXPConnect.cpp, line 592]
nsEventListenerManager::CompileEventHandlerInternal  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/content/events/src/nsEventListenerManager.cpp, line 1507]
nsEventListenerManager::HandleEventSubType  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/content/events/src/nsEventListenerManager.cpp, line 1643]
nsEventListenerManager::HandleEvent  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/content/events/src/nsEventListenerManager.cpp, line 1762]
nsGenericElement::HandleDOMEvent  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/content/base/src/nsGenericElement.cpp, line 2223]
nsGenericElement::HandleDOMEvent  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/content/base/src/nsGenericElement.cpp, line 2248]
nsEventStateManager::DispatchMouseEvent  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/content/events/src/nsEventStateManager.cpp, line 2687]
nsEventStateManager::NotifyMouseOver  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/content/events/src/nsEventStateManager.cpp, line 2812]
nsEventStateManager::GenerateMouseEnterExit  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/content/events/src/nsEventStateManager.cpp, line 2844]
nsEventStateManager::PreHandleEvent  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/content/events/src/nsEventStateManager.cpp, line 523]
PresShell::HandleEventInternal  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/layout/base/nsPresShell.cpp, line 6373]
PresShell::HandleEvent  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/layout/base/nsPresShell.cpp, line 6215]
nsViewManager::HandleEvent  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/view/src/nsViewManager.cpp, line 2559]
nsViewManager::DispatchEvent  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/view/src/nsViewManager.cpp, line 2246]
HandleEvent  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/view/src/nsView.cpp, line 174]
nsWindow::DispatchEvent  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/widget/src/windows/nsWindow.cpp, line 1349]
nsWindow::DispatchMouseEvent  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/widget/src/windows/nsWindow.cpp, line 6213]
ChildWindow::DispatchMouseEvent  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/widget/src/windows/nsWindow.cpp, line 6460]
nsWindow::WindowProc  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/widget/src/windows/nsWindow.cpp, line 1538]
USER32.dll + 0x8734 (0x77d18734)
USER32.dll + 0x8816 (0x77d18816)
USER32.dll + 0x89cd (0x77d189cd)
USER32.dll + 0x8a10 (0x77d18a10)
nsAppShell::Run  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/widget/src/windows/nsAppShell.cpp, line 159]
nsAppStartup::Run  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/toolkit/components/startup/src/nsAppStartup.cpp, line 152]
main  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/browser/app/nsBrowserApp.cpp, line 61]
kernel32.dll + 0x16d4f (0x7c816d4f)

Comment 2

[Martijn Wargers (dead)]

Ok, you need to do a little hovering over the document to get the crash.

Comment 3

[Daniel Veditz [:dveditz]]

I got a slightly different stack on a debug 1.5.0.5, a deleted (not null) "this" in nsIView::Destroy.

Comment 4

[Martijn Wargers (dead)]

Attachment: minimal testcase

Minimal testcase that crashes on branch with the same stacktrace.

Comment 5

[Martijn Wargers (dead)]

This was fixed on trunk between 2005-09-18 and 2005-09-19:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2005-09-18+07&maxdate=2005-09-19+09&cvsroot=%2Fcvsroot
Probably fixed by bug 11011.

Comment 6

[Daniel Veditz [:dveditz]]

I don't think we want bug 11011 on the 1.8 branch, let alone the 1.8.0 branch. There are still-open regressions and lots of complaints about layout incompatibilities which we're trying to avoid on the branches.

Is there a fix for the memory problem that doesn't side-step the issue by changing behavior like 11011?

Comment 7

[Boris Zbarsky [:bzbarsky]]

Hmm.. I wonder how that view ends up dying to start with...  I bet the problem is CantRenderReplacedElement, but I'm not sure how...

Comment 8

[Daniel Veditz [:dveditz]]

at risk for the 1.8.0 branch at least... is there a way to fix it without dragging in bug 11011? that one seems a bit too much risk to take.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed fix

What this does is make SplitToContainingBlock reparent views (like ConstructInlineFrame does).  We also Init the new frames with the right parents...

That seems to fix the crash for me; presumably the issue here was that that view of the rel pos fieldset ended up on the wrong view list and got destroyed before its frame did.

roc, dbaron, either one of you can probably r+sr this; just whichever gets to it first.

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 231144 [details] [diff] [review]
Proposed fix

>   // Create an "anonymous block" frame that will parent
>   // aBlockChildFrame. The new frame won't have a parent yet: the recursion
>   // will parent it.

This comment seems wrong now, as does the comment at the beginning of the function.

Why is the reparenting in the first part of the function (the base case of the recursion) insufficient?

Comment 11

[Boris Zbarsky [:bzbarsky]]

> This comment seems wrong now

Indeed.

> Why is the reparenting in the first part of the function (the base case of the
> recursion) insufficient?

It should be sufficient.  We still need to recurse to split ancestor inlines, of course...

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #11)
> > Why is the reparenting in the first part of the function (the base case of the
> > recursion) insufficient?
> 
> It should be sufficient.  We still need to recurse to split ancestor inlines,
> of course...

So the patch still works, but you don't understand why it works?

Comment 13

[Boris Zbarsky [:bzbarsky]]

It works for the reason stated in comment 9.

Perhaps I misunderstood the question in comment 10...  If you mean the existing nsHTMLContainerFrame::ReparentFrameViewList calls in the case when aFrame is not a block, those don't do what we want because ReparentFrameViewList doesn't descend into kids with views.  In this case our DOM looks like:

<object>
<object style="position: relative">
<object>
<fieldset style="position: relative"></fieldset>
</object>
</object>
</object>

and we call CantRenderReplacedElement on the objects starting from the outermost one.  When we do that for the innermost object, we discover that the fieldset is a block, so we have to split all three objects.  In the process, the fieldset ends up as a child of the blockframe for the innermost object, which is a child of the blockframe for the middle object.  The middle object's frames all have views, as does the fieldset, and we need to parent the fieldset's view to the view of the middle object's blockframe.  At the moment that's not happening, because the ReparentFrameViewList happens on the _outer_ object, and doesn't recurse into the middle object's views.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 231144 [details] [diff] [review]
Proposed fix

I believe it :-)

Comment 15

[Daniel Veditz [:dveditz]]

Is this patch appropriate for the 1.8.0 branch as well, or will it need a merged version?

Comment 16

[Boris Zbarsky [:bzbarsky]]

That patch applies to 1.8.0 as well.

Comment 17

[Mike Schroepfer]

Comment on attachment 231144 [details] [diff] [review]
Proposed fix

a=schrep for drivers.

Comment 18

[Daniel Veditz [:dveditz]]

Comment on attachment 231144 [details] [diff] [review]
Proposed fix

approved for 1.8.0 branch, a=dveditz for drivers

Comment 19

[Boris Zbarsky [:bzbarsky]]

Fixed on both branches.

Comment 20

[alice nodelman [:alice] [:anode]]

https://bugzilla.mozilla.org/attachment.cgi?id=228853&action=view should not crash.

Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1b2) Gecko/20060825 BonEcho/2.0b2

verified 1.8.1b2

Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.0.7pre) Gecko/20060825 Firefox/1.5.0.7pre

verified 1.8.0.7