372576 - crash [@ DoDeletingFrameSubtree] with textbox inside toolbarbutton

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[alex]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.2) Gecko/20070219 Firefox/2.0.0.2
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.2) Gecko/20070219 Firefox/2.0.0.2

In my extension i decided to put the textbox inside a toolbarbutton to save some space on my toolbar. The toolbarbutton is of type menu. In that menu i have the textbox and some menuitems. I have  onpopupshown="this.enableKeyboardNavigator(false);" on the menupopup in order to allow typing inside the textbox. Everything is ok i can open the menu, type inside the textbox and everything works (the actions associated with the textbox are all ok). The crash appears when closing the tab, the window or firefox.

Reproducible: Always

Steps to Reproduce:
1. Load the test XUL file in extension developer's XUL editor
2. press the button to open the menu of the toolbarbutton
3. close XUL editor
Actual Results:  
Firefox crashes. If you don't open the menu it wouldn't crash

Expected Results:  
Firefox should not have crashed

Used Firefox 2.0.0.2 on WindowsXP

Comment 1

[alex]

Attachment: The xul to reproduce the crash

Comment 2

[Steve England [:stevee]]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a3pre) Gecko/20070302 Minefield/3.0a3pre ID:2007030204 [cairo]

I believe this caused me to crash too. (after closing the testcase and trying to navigate away from bugzilla).

TB29906682Z

Incident ID: 29906682
Stack Signature	DoDeletingFrameSubtree 40d2dd49
Product ID	FirefoxTrunk
Build ID	2007030204
Trigger Time	2007-03-04 05:36:51.0
Platform	Win32
Operating System	Windows NT 5.0 build 2195
Module	firefox.exe + (0023dce2)
URL visited	bug 372576
User Comments	loaded bug 372576 testcase, clicked button, entered some text in text area, closed page, navigated away from bugzilla, crashed
Since Last Crash	58086 sec
Total Uptime	58086 sec
Trigger Reason	Access violation
Source File, Line No.	e:\builds\tinderbox\fx-trunk\winnt_5.2_depend\mozilla\layout\base\nscssframeconstructor.cpp, line 9247
Stack Trace 	
DoDeletingFrameSubtree  [mozilla/layout/base/nscssframeconstructor.cpp, line 9247]
DoDeletingFrameSubtree  [mozilla/layout/base/nscssframeconstructor.cpp, line 9253]
DoDeletingFrameSubtree  [mozilla/layout/base/nscssframeconstructor.cpp, line 9253]
DoDeletingFrameSubtree  [mozilla/layout/base/nscssframeconstructor.cpp, line 9253]
DoDeletingFrameSubtree  [mozilla/layout/base/nscssframeconstructor.cpp, line 9253]
DoDeletingFrameSubtree  [mozilla/layout/base/nscssframeconstructor.cpp, line 9253]
DoDeletingFrameSubtree  [mozilla/layout/base/nscssframeconstructor.cpp, line 9253]
DoDeletingFrameSubtree  [mozilla/layout/base/nscssframeconstructor.cpp, line 9253]
DeletingFrameSubtree  [mozilla/layout/base/nscssframeconstructor.cpp, line 9307]
nsCSSFrameConstructor::RemoveMappingsForFrameSubtree  [mozilla/layout/base/nscssframeconstructor.cpp, line 9331]
nsFrameList::DestroyFrames  [mozilla/layout/generic/nsframelist.cpp, line 60]
nsFrameList::DestroyFrames  [mozilla/layout/generic/nsframelist.cpp, line 60]
nsFrameList::DestroyFrames  [mozilla/layout/generic/nsframelist.cpp, line 60]
nsFrameList::DestroyFrames  [mozilla/layout/generic/nsframelist.cpp, line 60]
nsFrameList::DestroyFrames  [mozilla/layout/generic/nsframelist.cpp, line 60]
nsFrameManager::Destroy  [mozilla/layout/base/nsframemanager.cpp, line 301]
DocumentViewerImpl::Destroy  [mozilla/layout/base/nsdocumentviewer.cpp, line 1629]
nsSHEntry::~nsSHEntry  [mozilla/docshell/shistory/src/nsshentry.cpp, line 122]
nsSHEntry::Release  [mozilla/docshell/shistory/src/nsshentry.cpp, line 129]
nsDocShell::AddToSessionHistory  [mozilla/docshell/base/nsdocshell.cpp, line 7766]
nsDocShell::OnNewURI  [mozilla/docshell/base/nsdocshell.cpp, line 7536]
nsDocShell::OnLoadingSite  [mozilla/docshell/base/nsdocshell.cpp, line 7581]
nsDocShell::CreateContentViewer  [mozilla/docshell/base/nsdocshell.cpp, line 5757]
nsDSURIContentListener::DoContent  [mozilla/docshell/base/nsdsuricontentlistener.cpp, line 139]
nsDocumentOpenInfo::TryContentListener  [mozilla/uriloader/base/nsuriloader.cpp, line 791]
nsDocumentOpenInfo::DispatchContent  [mozilla/uriloader/base/nsuriloader.cpp, line 488]
table
0x7302f883

Dupe of bug 360339 and/or bug 364034?

Comment 3

[Mats Palmgren (inactive)]

It also crashes by:
 1. load the attached testcase
 2. press the button to open the menu of the toolbarbutton
 3. reload the page

Comment 4

[Mats Palmgren (inactive)]

Attachment: stack

Comment 5

[Mats Palmgren (inactive)]

Attachment: wallpaper

This fixes the crash for the testcase (and most likely bug 364034).
I think this is worth it, at least until we can figure out how to
make menu frame construction/destruction sane. Also for branches.

Comment 6

[Mats Palmgren (inactive)]

Attachment: Frame tree at the point of the crash

I think the frame tree is correct so this is something we need to deal with.

Comment 7

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

Deal with null out-of-flow, assert if not a popup.

Comment 8

[Boris Zbarsky [:bzbarsky]]

So why is this needed in this testcase?  Which subtree in that frametree do we enter twice and why?

Comment 9

[Mats Palmgren (inactive)]

We only enter DoDeletingFrameSubtree once. The popup frame is null because
the are only created while the are displayed (I think). I guess this is a
case DoDeletingFrameSubtree never really handled correctly; a menuitem
with a descendent popup (the textbox context menu in this case I think).

Comment 10

[Boris Zbarsky [:bzbarsky]]

Um...  if the frame is not created yet, why is the placeholder created?  I can't think of a way that should be able to happen.  In particular, the only caller of nsPlaceholderFrame() is NS_NewPlaceholderFrame(), the only caller of that is nsCSSFrameConstructor::CreatePlaceholderFrameFor, and that code would crash if aFrame were null.

So something is nulling out the out-of-flow on the placeholder without removing the placeholder from the tree.  Or we're crashing before it gets a chance to remove the placeholder from the tree...

Is that frame dump from a steady state? or sometime in the middle of teardown?

Comment 11

[Mats Palmgren (inactive)]

Right, there's something missing in my story, let me have a look again...

Comment 12

[Mats Palmgren (inactive)]

Attachment: Better story ;-)

(In reply to comment #10)
> So something is nulling out the out-of-flow on the placeholder without removing
> the placeholder from the tree. 

PresShell::Destroy
  nsFrameManager::Destroy
    nsFrameManager::ClearPlaceholderFrameMap

then while destroying the frame tree:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/xul/base/src/nsMenuFrame.cpp&rev=1.344&root=/cvsroot&mark=336#328

which is how we reach DoDeletingFrameSubtree() even though
PresShell.mIsDestroying=1

I see that PresShell::Destroy() calls mFrameConstructor->WillDestroyFrameTree()
before it starts destroying stuff, maybe we could use that to make an
early return in RemoveMappingsForFrameSubtree() ?
Or expose mIsDestroying?
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/base/nsPresShell.cpp&rev=3.971&root=/cvsroot&mark=1662#1618

Comment 13

[Boris Zbarsky [:bzbarsky]]

I'd prefer it if the frame constructor ignored calls to RemoveMappingsForFrameSubtree while we're destroying.  Either by asking the presshell, or by the presshell notifying the frame constructor when it starts destroying.

Comment 14

[Mats Palmgren (inactive)]

Attachment: Patch rev. 2

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 257266 [details] [diff] [review]
Patch rev. 2

Looks good.  Add a comment saying that the tree might not be in a consistent state to start with if we're destroying the presshell or something?

Comment 17

[Mats Palmgren (inactive)]

Added a comment and checked in to trunk at 2007-03-04 14:39 PST.
Will ask for branch approval in a couple of days.

-> FIXED

Comment 18

[Boris Zbarsky [:bzbarsky]]

You can't change nsIPresShell on branch.

Comment 20

[Mats Palmgren (inactive)]

Attachment: Alternative patch for branches

Would this be an acceptable change for the branches?

Comment 21

[Boris Zbarsky [:bzbarsky]]

Er, I somehow missed part of comment 12.  That patch is fine for branches, and in fact is what we should do on trunk, imo.

Comment 22

[Mike Connor [:mconnor]]

not in scope for 1.8.1.3, moving nom to 1.8.1.4

Comment 23

[Mats Palmgren (inactive)]

Attachment: Alternative patch, for trunk and branches

Note: this is a 1.8 branch diff.
For trunk I'll back out rev. 2 in addition to this.
(I changed the comment slightly compared with the last "Alternative
patch" - no code changes.)

Comment 24

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 257501 [details] [diff] [review]
Alternative patch, for trunk and branches

Looks great!

Comment 25

[Mats Palmgren (inactive)]

Comment on attachment 257501 [details] [diff] [review]
Alternative patch, for trunk and branches

Landed on trunk (+ backing out rev. 2) at 2007-03-06 13:07 PST.

Comment 26

[Mats Palmgren (inactive)]

Comment on attachment 257501 [details] [diff] [review]
Alternative patch, for trunk and branches

Low-risk crash fix

Comment 27

[Daniel Veditz [:dveditz]]

Comment on attachment 257501 [details] [diff] [review]
Alternative patch, for trunk and branches

approved for 1.8/1.8.0 branches, a=dveditz for drivers

Comment 28

[Mats Palmgren (inactive)]

Checked in to MOZILLA_1_8_BRANCH at 2007-03-23 00:25 PST
Checked in to MOZILLA_1_8_0_BRANCH at 2007-03-23 00:38 PST

Comment 29

[Martijn Wargers (dead)]

Verified fixed on branch. The latest branch builds don't crash anymore on the testcase and the steps to reproduce.

Comment 30

[Mats Palmgren (inactive)]

Crash test:
https://hg.mozilla.org/integration/mozilla-inbound/rev/499f187c9b7a

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/499f187c9b7a