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

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...

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...

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

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...

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

* 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

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.

Comment on attachment 284978 [details] [diff] [review] Patch rev. 1 ok, I'll fix that before checkin

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: http://crash-stats.mozilla.com/report/index/fd7d6f30-8171-11dc-a531-001a4bd46e84

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

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.

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

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.