See testcase, which crashes current trunk build on load. Talkback ID: TB29940125X DoDeletingFrameSubtree [mozilla/layout/base/nscssframeconstructor.cpp, line 9270] DoDeletingFrameSubtree [mozilla/layout/base/nscssframeconstructor.cpp, line 9296] DeletingFrameSubtree [mozilla/layout/base/nscssframeconstructor.cpp, line 9350] nsCSSFrameConstructor::RemoveMappingsForFrameSubtree [mozilla/layout/base/nscssframeconstructor.cpp, line 9381] nsBoxFrame::RemoveFrame [mozilla/layout/xul/base/src/nsboxframe.cpp, line 1031] nsFrameManager::RemoveFrame [mozilla/layout/base/nsframemanager.cpp, line 711] nsCSSFrameConstructor::ContentRemoved [mozilla/layout/base/nscssframeconstructor.cpp, line 9554] nsCSSFrameConstructor::RecreateFramesForContent [mozilla/layout/base/nscssframeconstructor.cpp, line 11113] nsCSSFrameConstructor::RestyleElement [mozilla/layout/base/nscssframeconstructor.cpp, line 9992] 0x02a5d240

Frame tree when we get a call to DeletingFrameSubtree(0x12be430): Viewport(-1)@0xa526d8 ... < RootBox(window)(-1)@0xa527e0 ... < DocElementBox(window)(-1)@0xa52d28 ... < PopupSet(popupgroup)(-1)@0xa52eb8 ... <> Placeholder(tooltip)(-1)@0x12bdde0 ... <> Menu(menuitem)(0)@0x12be430 {0,0,73920,240} Popup-list< MenuPopup(box)(0)@0x12be738 ... < Placeholder(box)(0)@0x12c5230 outOfFlowFrame=Area(box)(0)@0x12c50e8 > > > > > Fixed-list< Area(box)(0)@0x12c50e8 ...<> > we find the placeholder 0x12c5230 and put the fixed frame 0x12c50e8 in the destroy queue and eventually destroy it. Back in ContentRemoved() we remove the frame 0x12be430 which will eventually call DeletingFrameSubtree() through RemoveMappingsForFrameSubtree() on its Popup-list frames... so we find the placeholder again, which now has a mOutOfFlowFrame pointing to a destroyed frame of course... My first thought was to add "nsGkAtoms::popupList" to the list here: http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/base/nsCSSFrameConstructor.cpp&rev=1.1323&root=/cvsroot&mark=9291-9298#9252 It fixes the crash, but I need to think about this a bit...

That would make some sense, as we're making popups more and more like normal out-of-flows...

Making all sg:critical 1.9 blockers.

Poke. Mats, news?

The testcase is worksforme with current trunk build. Not sure if that says anything. Can this bug be closed?

Yeah, I think this is correct as is actually. The Popup-list is only exposed by nsMenuFrame, where the frames are in-flow and thus *should* be traversed DeletingFrameSubtree(). The reason for the crash was that RemoveMappingsForFrameSubtree() was called from nsMenuFrame::DestroyPopupFrames(), but that has now been removed (I'm guessing from Neil's menu refactoring). http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/xul/base/src/nsMenuFrame.cpp&rev=1.347&root=/cvsroot&mark=340#331 We still have the out-of-flow popup frames in a popupset to worry about, but nsPopupSetFrame does not expose its Popup-list to the frame constructor although you can add/remove frames to the Popup-list: http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/xul/base/src/nsPopupSetFrame.cpp&rev=1.170&root=/cvsroot&mark=111,121,132,142#107 it doesn't actually give a frame back when calling GetFirstFrame(popupList), nor does it have GetAdditionalChildListName(). These frames are managed internally by nsPopupSetFrame so I guess that's ok, but when nsPopupSetFrame destroys these frames it doesn't call RemoveMappingsForFrameSubtree(), which makes me wonder: 1. Who does the frame manager notifications that DoDeletingFrameSubtree() normally does? (i.e. RemoveAsPrimaryFrame/ClearAllUndisplayedContentIn) 2. How are fixed pos frames inside such a popup handled? if they can escape outside (fixed pos) that could cause a crash...(I did a quick test and it seems we don't create out-of-flows at all, which is good I guess, but I didn't run any extensive tests...) I don't know how severe 1. is, if it's not actually done. Boris?

Not doing 1 could lead to effective memory leaks for the lifetime of the page, and could lead to GetPrimaryFrameFor returning dead frames. The latter is bad, of course.

Ok, I guess we should add back a RemoveMappingsForFrameSubtree() for the frames that nsPopupSetFrame destroys then. I'll have a look...

layout/xul/Makefile.in: Adding layout/xul/test directory for mochitest. nsMenuPopupFrame.cpp: Just fixing a couple of compile warnings: GetPopupSetFrame() and rootView are unused. nsPopupSetFrame.cpp/h: Call RemoveMappingsForFrameSubtree() before destroying an "internal" popup frame. Remove unnecessary #includes.

Comment on attachment 279236 [details] [diff] [review] Patch rev. 1 Looks reasonable

Mats, can we get this landed and resolved?

Mats, ping?

mozilla/layout/xul/Makefile.in 1.9 mozilla/layout/xul/test/Makefile.in 1.1 mozilla/layout/xul/test/test_bug372685.xul 1.1 mozilla/layout/xul/base/src/nsMenuPopupFrame.cpp 1.330 mozilla/layout/xul/base/src/nsPopupSetFrame.cpp 1.171 mozilla/layout/xul/base/src/nsPopupSetFrame.h 1.61 -> FIXED