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

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Martijn Wargers (dead)]

Attachment: testcase

See testcase, which crashes current trunk build after 200ms.
This regressed between 2007-10-05 and 2007-10-07:
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=2007-10-05+04&maxdate=2007-10-07+09&cvsroot=%2Fcvsroot
Somehow a regression from bug 372685?

http://crash-stats.mozilla.com/report/index/4c3f3d97-7591-11dc-94e2-001a4bd43e5c
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

Comment 1

[Mats Palmgren (inactive)]

Attachment: trace + frame dump

ReconstructDocElementHierarchyInternal() clears the frame manager maps
before destroying the frame tree:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/base/nsCSSFrameConstructor.cpp&rev=1.1408&root=/cvsroot&mark=7724-7728,7739#7691
ClearPlaceholderFrameMap() removes all the placeholder/out-of-flow mappings
so DoDeletingFrameSubtree() will eventually crash on a null out-of-flow:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/base/nsCSSFrameConstructor.cpp&rev=1.1408&root=/cvsroot&mark=9284#9247

RemoveMappingsForFrameSubtree() has a check to prevent this:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/base/nsCSSFrameConstructor.cpp&rev=1.1408&root=/cvsroot&mark=9367#9364
but |mIsDestroyingFrameTree| is false in this case...

Comment 2

[Mats Palmgren (inactive)]

Attachment: wip

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?
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/base/nsCSSFrameConstructor.cpp&rev=1.1408&root=/cvsroot&mark=10190#10183
more later...

Comment 3

[Boris Zbarsky [:bzbarsky]]

Yes, clearing quotes, counters, and pending restyles here would be a really good idea.

Comment 4

[Mats Palmgren (inactive)]

Attachment: Testcase #2

Comment 5

[Mats Palmgren (inactive)]

Attachment: trace + frame dump (for 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...

Comment 6

[Mats Palmgren (inactive)]

AFAICT, a popupset frame's ::popupList will only contain out-of-flow
MenuPopupFrames.
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/base/nsCSSFrameConstructor.cpp&rev=1.1410&root=/cvsroot&mark=6028-6031,6045,6100,6147#6024
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/base/nsCSSFrameConstructor.cpp&rev=1.1410&root=/cvsroot&mark=1366,1370,1371,1399,1404#1348

Comment 7

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

* 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

Comment 8

[Mats Palmgren (inactive)]

Attachment: Mochitests

The attached testcases in Mochitest form.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 284978 [details] [diff] [review]
Patch rev. 1

DEBUG, not NS_DEBUG, please.  r+sr=bzbarsky with that.

Comment 10

[Mats Palmgren (inactive)]

Comment on attachment 284978 [details] [diff] [review]
Patch rev. 1

ok, I'll fix that before checkin

Comment 11

[Martijn Wargers (dead)]

Fyi, the patch doesn't need approval, because this bug is blockg1.9+, so you should be ale to check it in.

Comment 12

[Boris Zbarsky [:bzbarsky]]

That's no longer the case, at least till after beta.  It needs approval now to land for beta1.

Comment 13

[Martijn Wargers (dead)]

I think I also get this crash while playing around with the search input in the Places Organizer:
http://crash-stats.mozilla.com/report/index/fd7d6f30-8171-11dc-a531-001a4bd46e84

Comment 14

[Mats Palmgren (inactive)]

Looks like it.  Maybe we should take this for beta1 then?

Comment 15

[Damon Sicore (:damons)]

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 16

[Damon Sicore (:damons)]

Comment on attachment 284978 [details] [diff] [review]
Patch rev. 1

Approved to land for beta.  Please check in ASAP.

Comment 17

[Mats Palmgren (inactive)]

Attachment: patch with nit fixed + mochitests (what was checked in)

Comment 18

[Mats Palmgren (inactive)]

mozilla/layout/xul/test/Makefile.in 	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 

-> FIXED

Comment 19

[Marcia Knous [:marcia]]

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.