Closed Bug 398982 Opened 13 years ago Closed 13 years ago

Crash [@ DoDeletingFrameSubtree] with menuitem, tooltip, treecols and removing position: absolute


(Core :: Layout, defect)

Not set





(Reporter: martijn.martijn, Assigned: mats)



(Keywords: crash, regression, testcase, Whiteboard: [dbaron-1.9:Rs])

Crash Data


(7 files, 1 obsolete file)

Attached file testcase
See testcase, which crashes current trunk build after 200ms.
This regressed between 2007-10-05 and 2007-10-07:
Somehow a regression from bug 372685?
0  	DoDeletingFrameSubtree  	 mozilla/layout/base/nsCSSFrameConstructor.cpp:9285
1 	DoDeletingFrameSubtree 	mozilla/layout/base/nsCSSFrameConstructor.cpp:9291
2 	DoDeletingFrameSubtree 	mozilla/layout/base/nsCSSFrameConstructor.cpp:9291
3 	DeletingFrameSubtree 	mozilla/layout/base/nsCSSFrameConstructor.cpp:9339
4 	nsCSSFrameConstructor::RemoveMappingsForFrameSubtree(nsIFrame*) 	mozilla/layout/base/nsCSSFrameConstructor.cpp:9378
5 	nsPopupSetFrame::DestroyPopupFrame(nsCSSFrameConstructor*, nsIFrame*) 	mozilla/layout/xul/base/src/nsPopupSetFrame.cpp:303
6 	nsPopupSetFrame::Destroy() 	mozilla/layout/xul/base/src/nsPopupSetFrame.cpp:147
7 	nsFrameList::DestroyFrames() 	mozilla/layout/generic/nsFrameList.cpp:67
8 	nsContainerFrame::Destroy() 	mozilla/layout/generic/nsContainerFrame.cpp:259
9 	nsBoxFrame::RemoveFrame(nsIAtom*, nsIFrame*) 	mozilla/layout/xul/base/src/nsBoxFrame.cpp:1012
10 	nsRootBoxFrame::RemoveFrame(nsIAtom*, nsIFrame*) 	mozilla/layout/xul/base/src/nsRootBoxFrame.cpp:212
11 	nsFrameManager::RemoveFrame(nsIFrame*, nsIAtom*, nsIFrame*) 	mozilla/layout/base/nsFrameManager.cpp:690
12 	nsCSSFrameConstructor::ReconstructDocElementHierarchyInternal() 	mozilla/layout/base/nsCSSFrameConstructor.cpp:7740
13 	nsCSSFrameConstructor::RecreateFramesForContent(nsIContent*) 	mozilla/layout/base/nsCSSFrameConstructor.cpp:11120
14 	nsCSSFrameConstructor::RestyleElement(nsIContent*, nsIFrame*, nsChangeHint) 	mozilla/layout/base/nsCSSFrameConstructor.cpp:9973
15 	nsCSSFrameConstructor::ProcessOneRestyle(nsIContent*, nsReStyleHint, nsChangeHint) 	mozilla/layout/base/nsCSSFrameConstructor.cpp:13001
16 	nsCSSFrameConstructor::ProcessPendingRestyles() 	mozilla/layout/base/nsCSSFrameConstructor.cpp:13054
17 	PresShell::DoFlushPendingNotifications(mozFlushType, int) 	mozilla/layout/base/nsPresShell.cpp:4443
18 	PresShell::FlushPendingNotifications(mozFlushType) 	mozilla/layout/base/nsPresShell.cpp:4407
19 	nsCSSFrameConstructor::RestyleEvent::Run() 	mozilla/layout/base/nsCSSFrameConstructor.cpp:13110
20 	nsThread::ProcessNextEvent(int, int*) 	mozilla/xpcom/threads/nsThread.cpp:490
21 	NS_ProcessNextEvent_P(nsIThread*, int) 	nsThreadUtils.cpp:227
22 	nsBaseAppShell::Run() 	mozilla/widget/src/xpwidgets/nsBaseAppShell.cpp:154
23 	nsAppStartup::Run() 	mozilla/toolkit/components/startup/src/nsAppStartup.cpp:170
24 	XRE_main 	mozilla/toolkit/xre/nsAppRunner.cpp:3142
25 	main 	mozilla/browser/app/nsBrowserApp.cpp:153
26 	WinMain 	mozilla/browser/app/nsBrowserApp.cpp:166
27 	__tmainCRTStartup 	crtexe.c:589
28 	BaseProcessStart
Assignee: nobody → mats.palmgren
Blocks: 372685
Flags: blocking1.9?
OS: Windows XP → All
Hardware: PC → All
Attached file trace + frame dump
ReconstructDocElementHierarchyInternal() clears the frame manager maps
before destroying the frame tree:,7739#7691
ClearPlaceholderFrameMap() removes all the placeholder/out-of-flow mappings
so DoDeletingFrameSubtree() will eventually crash on a null out-of-flow:

RemoveMappingsForFrameSubtree() has a check to prevent this:
but |mIsDestroyingFrameTree| is false in this case...
Attached patch wip (obsolete) — Splinter Review
This patch fixes the crash, but maybe we should call WillDestroyFrameTree()
instead, to clear counters and pending restyles - there's no point in
keeping those if we recreate the entire frame tree is there?
more later...
Yes, clearing quotes, counters, and pending restyles here would be a really good idea.
Attached file Testcase #2
There is actually a problem with the unconditional
RemoveMappingsForFrameSubtree() that we added in bug 372685.
This is the frame dump + trace for Testcase #2.  This time
ReconstructDocElementHierarchyInternal() is not involved at all, it's a
restyle causing RecreateFramesForContent().  See the original frame tree
before it begins at the top, and the stack at the bottom.

Note that all popup-list children are out-of-flows (red,blue,pink).
We should not call RemoveMappingsForFrameSubtree() for out-of-flows when
the placeholder is on a reachable frame list since we would traverse the
same frames twice (same as DoDeletingFrameSubtree() avoids those lists).

-  aFc->RemoveMappingsForFrameSubtree(aPopup);
+  if (!(aPopup->GetStateBits() & NS_FRAME_OUT_OF_FLOW))
+    aFc->RemoveMappingsForFrameSubtree(aPopup);

would do the right thing, but...

After reading the frame construction code, it looks like a PopupSet's
::popupList can only contain out-of-flows which means we don't need to call
RemoveMappingsForFrameSubtree() at all here since all these frames will be
reachable (directly or indirectly) through a placeholder from outside the
::popupList itself. So, I'm leaning towards removing it again and adding
assertions instead... I'll investigate some more...
Attached patch Patch rev. 1Splinter Review
* call WillDestroyFrameTree() before destroying the frame tree
* revert the patch from bug 372685 and add assertions that the ::popupList
  frames must be out-of-flow MenuPopupFrames
* add nsPopupSetFrame::List() that also prints the ::popupList frames
Attachment #284041 - Attachment is obsolete: true
Attachment #284978 - Flags: superreview?(bzbarsky)
Attachment #284978 - Flags: review?(bzbarsky)
Attached patch MochitestsSplinter Review
The attached testcases in Mochitest form.
Comment on attachment 284978 [details] [diff] [review]
Patch rev. 1

DEBUG, not NS_DEBUG, please.  r+sr=bzbarsky with that.
Attachment #284978 - Flags: superreview?(bzbarsky)
Attachment #284978 - Flags: superreview+
Attachment #284978 - Flags: review?(bzbarsky)
Attachment #284978 - Flags: review+
Comment on attachment 284978 [details] [diff] [review]
Patch rev. 1

ok, I'll fix that before checkin
Attachment #284978 - Flags: approval1.9?
Flags: blocking1.9? → blocking1.9+
Whiteboard: [dbaron-1.9:Rs]
Fyi, the patch doesn't need approval, because this bug is blockg1.9+, so you should be ale to check it in.
That's no longer the case, at least till after beta.  It needs approval now to land for beta1.
I think I also get this crash while playing around with the search input in the Places Organizer:
Looks like it.  Maybe we should take this for beta1 then?
Target Milestone: --- → mozilla1.9 M9
Should this definitely block beta?  Criteria to use to judge:

* Most sites should display properly and regression free (from previous
major release)
* No known common data loss bugs
* No common hangs or crashes
* No problems with major features in common use cases

Also, what's the risk of regression w/ this patch?

Thanks for any clarification here.  
Comment on attachment 284978 [details] [diff] [review]
Patch rev. 1

Approved to land for beta.  Please check in ASAP.
Attachment #284978 - Flags: approval1.9? → approval1.9+
mozilla/layout/xul/test/ 	1.4
mozilla/layout/xul/test/test_bug398982-1.xul 	1.1
mozilla/layout/xul/test/test_bug398982-2.xul 	1.1
mozilla/layout/base/nsCSSFrameConstructor.cpp 	1.1417
mozilla/layout/xul/base/src/nsPopupSetFrame.cpp 	1.172
mozilla/layout/xul/base/src/nsPopupSetFrame.h 	1.62 

Closed: 13 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
verified fixed using Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9a9pre) Gecko/2007102407 Minefield/3.0a9pre and Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.9a9pre) Gecko/2007102404 Minefield/3.0a9pre. I ran both testcases in the bug and no crash was observed.
Crash Signature: [@ DoDeletingFrameSubtree]
You need to log in before you can comment on or make changes to this bug.