Last Comment Bug 344291 - [FIX]Crash [@ nsIView::Destroy] on 1.8.0.5 and 1.8.1 branch
: [FIX]Crash [@ nsIView::Destroy] on 1.8.0.5 and 1.8.1 branch
Status: VERIFIED FIXED
[sg:critical] [at risk] fixed on trun...
: crash, verified1.8.0.7, verified1.8.1
Product: Core
Classification: Components
Component: Layout (show other bugs)
: 1.8 Branch
: x86 Windows XP
: P1 normal (vote)
: mozilla1.8.1
Assigned To: Boris Zbarsky [:bz]
:
Mentors:
Depends on:
Blocks: 344056 348470
  Show dependency treegraph
 
Reported: 2006-07-11 14:45 PDT by Martijn Wargers [:mwargers] (not working for Mozilla)
Modified: 2011-06-13 10:01 PDT (History)
7 users (show)
darin.moz: blocking1.8.1+
dveditz: blocking1.8.0.7+
dveditz: in‑testsuite?
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
minimal testcase (419 bytes, text/html)
2006-07-24 00:25 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
Proposed fix (4.73 KB, patch)
2006-07-28 13:43 PDT, Boris Zbarsky [:bz]
roc: review+
dbaron: superreview+
dveditz: approval1.8.0.7+
mtschrep: approval1.8.1+
Details | Diff | Splinter Review

Description Martijn Wargers [:mwargers] (not working for Mozilla) 2006-07-11 14:45:17 PDT
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 [:mwargers] (not working for Mozilla) 2006-07-11 14:46:42 PDT
Ok, you need to do a little hovering over the document to get the crash.
Comment 3 Daniel Veditz [:dveditz] 2006-07-13 16:46:55 PDT
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 [:mwargers] (not working for Mozilla) 2006-07-24 00:25:02 PDT
Created attachment 230401 [details]
minimal testcase

Minimal testcase that crashes on branch with the same stacktrace.
Comment 6 Daniel Veditz [:dveditz] 2006-07-24 16:47:18 PDT
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 [:bz] 2006-07-24 19:16:29 PDT
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] 2006-07-27 17:35:51 PDT
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 [:bz] 2006-07-28 13:43:02 PDT
Created attachment 231144 [details] [diff] [review]
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+2 (mostly busy through August 4; review requests must explain patch) 2006-07-28 14:15:44 PDT
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 [:bz] 2006-07-28 18:15:11 PDT
> 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+2 (mostly busy through August 4; review requests must explain patch) 2006-08-01 14:19:12 PDT
(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 [:bz] 2006-08-01 14:36:50 PDT
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) (Exited; email my personal email if necessary) 2006-08-06 15:11:40 PDT
Comment on attachment 231144 [details] [diff] [review]
Proposed fix

I believe it :-)
Comment 15 Daniel Veditz [:dveditz] 2006-08-23 15:18:41 PDT
Is this patch appropriate for the 1.8.0 branch as well, or will it need a merged version?
Comment 16 Boris Zbarsky [:bz] 2006-08-23 17:37:36 PDT
That patch applies to 1.8.0 as well.
Comment 17 Mike Schroepfer 2006-08-24 10:35:05 PDT
Comment on attachment 231144 [details] [diff] [review]
Proposed fix

a=schrep for drivers.
Comment 18 Daniel Veditz [:dveditz] 2006-08-24 11:28:59 PDT
Comment on attachment 231144 [details] [diff] [review]
Proposed fix

approved for 1.8.0 branch, a=dveditz for drivers
Comment 19 Boris Zbarsky [:bz] 2006-08-24 16:18:28 PDT
Fixed on both branches.
Comment 20 alice nodelman [:alice] [:anode] 2006-08-25 17:12:05 PDT
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

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