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

VERIFIED FIXED in mozilla1.8.1

Status

()

Core
Layout
P1
normal
VERIFIED FIXED
11 years ago
6 years ago

People

(Reporter: Martijn Wargers (dead), Assigned: bz)

Tracking

({crash, verified1.8.0.7, verified1.8.1})

1.8 Branch
mozilla1.8.1
x86
Windows XP
crash, verified1.8.0.7, verified1.8.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8.1 +
blocking1.8.0.7 +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical] [at risk] fixed on trunk by 11011?, crash signature)

Attachments

(2 attachments)

(Reporter)

Description

11 years ago
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)
(Reporter)

Comment 2

11 years ago
Ok, you need to do a little hovering over the document to get the crash.
I got a slightly different stack on a debug 1.5.0.5, a deleted (not null) "this" in nsIView::Destroy.
Flags: blocking1.8.1?
Flags: blocking1.8.0.6?
Whiteboard: [sg:critical] fixed on trunk?
Version: Trunk → 1.8 Branch

Updated

11 years ago
Flags: blocking1.8.1? → blocking1.8.1+
(Reporter)

Comment 4

11 years ago
Created attachment 230401 [details]
minimal testcase

Minimal testcase that crashes on branch with the same stacktrace.
(Reporter)

Comment 5

11 years ago
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.
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?
Whiteboard: [sg:critical] fixed on trunk? → [sg:critical] fixed on trunk by 11011?
(Assignee)

Comment 7

11 years ago
Hmm.. I wonder how that view ends up dying to start with...  I bet the problem is CantRenderReplacedElement, but I'm not sure how...
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.
Assignee: nobody → bzbarsky
Flags: blocking1.8.0.6? → blocking1.8.0.6+
Whiteboard: [sg:critical] fixed on trunk by 11011? → [sg:critical] [at risk] fixed on trunk by 11011?
(Assignee)

Comment 9

11 years ago
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.
Attachment #231144 - Flags: superreview?(dbaron)
Attachment #231144 - Flags: review?(roc)
(Assignee)

Updated

11 years ago
Priority: -- → P1
Summary: 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
Target Milestone: --- → mozilla1.8.1beta2
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?
(Assignee)

Comment 11

11 years ago
> 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...
(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?
(Assignee)

Comment 13

11 years ago
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 on attachment 231144 [details] [diff] [review]
Proposed fix

I believe it :-)
Attachment #231144 - Flags: review?(roc) → review+

Updated

11 years ago
Target Milestone: mozilla1.8.1beta2 → mozilla1.8.1
Whiteboard: [sg:critical] [at risk] fixed on trunk by 11011? → [sg:critical] need sr=dbaron [at risk] fixed on trunk by 11011?
Attachment #231144 - Flags: superreview?(dbaron) → superreview+
Is this patch appropriate for the 1.8.0 branch as well, or will it need a merged version?
Whiteboard: [sg:critical] need sr=dbaron [at risk] fixed on trunk by 11011? → [sg:critical] [at risk] fixed on trunk by 11011?
(Assignee)

Updated

11 years ago
Attachment #231144 - Flags: approval1.8.1?
Attachment #231144 - Flags: approval1.8.0.7?
(Assignee)

Comment 16

11 years ago
That patch applies to 1.8.0 as well.

Comment 17

11 years ago
Comment on attachment 231144 [details] [diff] [review]
Proposed fix

a=schrep for drivers.
Attachment #231144 - Flags: approval1.8.1? → approval1.8.1+
Comment on attachment 231144 [details] [diff] [review]
Proposed fix

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #231144 - Flags: approval1.8.0.7? → approval1.8.0.7+
(Assignee)

Comment 19

11 years ago
Fixed on both branches.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Keywords: fixed1.8.0.7, fixed1.8.1
Resolution: --- → FIXED
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
Status: RESOLVED → VERIFIED
Keywords: fixed1.8.0.7, fixed1.8.1 → verified1.8.0.7, verified1.8.1
Keywords: crash
(Reporter)

Updated

11 years ago
Blocks: 348470
Group: security
Flags: in-testsuite?
Crash Signature: [@ nsIView::Destroy]
You need to log in before you can comment on or make changes to this bug.