crash [@ DoDeletingFrameSubtree] with textbox inside toolbarbutton

RESOLVED FIXED

Status

()

defect
--
critical
RESOLVED FIXED
13 years ago
7 years ago

People

(Reporter: atg2dg, Assigned: mats)

Tracking

(4 keywords)

unspecified
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8.1.3 -
blocking1.8.1.4 +
blocking1.8.0.12 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(crash signature)

Attachments

(5 attachments, 4 obsolete attachments)

Reporter

Description

13 years ago
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
Reporter

Comment 1

13 years ago
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?
Keywords: crash, testcase
Assignee

Comment 3

13 years ago
It also crashes by:
 1. load the attached testcase
 2. press the button to open the menu of the toolbarbutton
 3. reload the page
Status: UNCONFIRMED → NEW
Component: Menus → Layout
Ever confirmed: true
OS: Windows XP → All
Product: Firefox → Core
QA Contact: menus → layout
Hardware: PC → All
Summary: crash with textbox inside toolbarbutton → crash [@ DoDeletingFrameSubtree] with textbox inside toolbarbutton
Assignee

Comment 4

13 years ago
Posted file stack
Assignee

Comment 5

13 years ago
Posted patch wallpaper (obsolete) — Splinter Review
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.
Assignee

Comment 6

13 years ago
I think the frame tree is correct so this is something we need to deal with.
Assignee: nobody → mats.palmgren
Attachment #257225 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Assignee

Comment 7

13 years ago
Posted patch Patch rev. 1 (obsolete) — Splinter Review
Deal with null out-of-flow, assert if not a popup.
Attachment #257235 - Flags: superreview?(bzbarsky)
Attachment #257235 - Flags: review?(bzbarsky)
Assignee

Updated

13 years ago
Flags: blocking1.9?
Flags: blocking1.8.1.3?
Flags: blocking1.8.0.11?
So why is this needed in this testcase?  Which subtree in that frametree do we enter twice and why?
Assignee

Comment 9

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

Comment 11

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

Comment 12

13 years ago
Posted file 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
Attachment #257235 - Attachment is obsolete: true
Attachment #257235 - Flags: superreview?(bzbarsky)
Attachment #257235 - Flags: review?(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.
Assignee

Comment 14

13 years ago
Posted patch Patch rev. 2 (obsolete) — Splinter Review
Attachment #257266 - Flags: superreview?(bzbarsky)
Attachment #257266 - Flags: review?(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?
Attachment #257266 - Flags: superreview?(bzbarsky)
Attachment #257266 - Flags: superreview+
Attachment #257266 - Flags: review?(bzbarsky)
Attachment #257266 - Flags: review+
Assignee

Updated

13 years ago
Blocks: 360339
Assignee

Updated

13 years ago
Duplicate of this bug: 350460
Assignee

Comment 17

13 years ago
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
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Flags: blocking1.9?
Resolution: --- → FIXED
Assignee

Updated

13 years ago
Blocks: 364034
You can't change nsIPresShell on branch.
Assignee

Updated

13 years ago
Duplicate of this bug: 367149
Assignee

Comment 20

13 years ago
Would this be an acceptable change for the branches?
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.
not in scope for 1.8.1.3, moving nom to 1.8.1.4
Flags: blocking1.8.1.4?
Flags: blocking1.8.1.3?
Flags: blocking1.8.1.3-
Assignee

Comment 23

13 years ago
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.)
Attachment #257286 - Attachment is obsolete: true
Attachment #257501 - Flags: superreview?(bzbarsky)
Attachment #257501 - Flags: review?(bzbarsky)
Comment on attachment 257501 [details] [diff] [review]
Alternative patch, for trunk and branches

Looks great!
Attachment #257501 - Flags: superreview?(bzbarsky)
Attachment #257501 - Flags: superreview+
Attachment #257501 - Flags: review?(bzbarsky)
Attachment #257501 - Flags: review+
Assignee

Comment 25

13 years ago
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.
Assignee

Updated

13 years ago
Attachment #257266 - Attachment is obsolete: true

Updated

13 years ago
Flags: in-testsuite?
Assignee

Comment 26

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

Low-risk crash fix
Attachment #257501 - Flags: approval1.8.1.4?
Attachment #257501 - Flags: approval1.8.0.12?
Flags: blocking1.8.1.4?
Flags: blocking1.8.1.4+
Flags: blocking1.8.0.12?
Flags: blocking1.8.0.12+
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
Attachment #257501 - Flags: approval1.8.1.4?
Attachment #257501 - Flags: approval1.8.1.4+
Attachment #257501 - Flags: approval1.8.0.12?
Attachment #257501 - Flags: approval1.8.0.12+
Assignee

Comment 28

12 years ago
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
Blocks: 325377
Verified fixed on branch. The latest branch builds don't crash anymore on the testcase and the steps to reproduce.
Crash Signature: [@ DoDeletingFrameSubtree]
Assignee

Comment 30

7 years ago
Crash test:
https://hg.mozilla.org/integration/mozilla-inbound/rev/499f187c9b7a
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.