Johnny Stenback (:jst) Reporter
	Description • 23 years ago

When loading the above page, I hit the assertion in
FrameManager::NotifyDestroyingFrame() about the frame not being removed
propertly from the primary frame map. Seems bad...

	David Baron :dbaron:
	Comment 1 • 23 years ago

There is another bug on seeing that with object frames that get replaced.

	Bernd
	Comment 2 • 23 years ago

probably a dupe of bug 101216

	David Baron :dbaron:
	Comment 3 • 23 years ago

No, I was thinking of bug 117703.

	Daniel Bratell
	Comment 4 • 23 years ago

The assertions are kind of annoying. Is there a simple fix for whatever bug this 
is?

	David Baron :dbaron:
	Comment 5 • 23 years ago

The assertions are a sign that we're very likely to crash if we do anything else
with the page, and they should not be removed.  What we could do is change the
code so that it does the removal there -- I've been tempted to do that, but I
don't want to have the removal in both places, and I'm not sure what would
happen if we removed it from the earlier site.

	Daniel Bratell
	Comment 6 • 23 years ago

Yes, I don't want the assertion code gone, just a fix so that the assertion
isn't triggered. 

Why isn't the frame remove the same way for objects as for everything else?
Plugin difficulties?

	rbs
	Comment 7 • 23 years ago

Another page where the assertion is a always firing: 
http://news.bbc.co.uk/sport3/worldcup2002/hi/history/newsid_1966000/1966379.stm

	Mats Palmgren (inactive) Assignee
	Comment 8 • 19 years ago

Call 'RemoveMappingsForFrameSubtree()' before destroying 'mPopupFrames'.

	Boris Zbarsky [:bzbarsky]
	Comment 9 • 19 years ago

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

r+sr=bzbarsky, but perhaps nsFrameList should just handle this?  Or would this
a very special case?

	Mats Palmgren (inactive) Assignee
	Comment 10 • 19 years ago

I did a quick investigation into why this is needed.
The assertion is rooted in call to WipeContainingBlock():
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/base/nsCSSFrameConstructor.cpp&rev=1.1126&root=/cvsroot&mark=13176#13135

it seems that WipeContainingBlock() takes care of abs/fixed/float through the
state and that CleanupFrameReferences() is designed to only take care of the
principal child list.
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/base/nsCSSFrameConstructor.cpp&rev=1.1126&root=/cvsroot&mark=682#652

This means that any frame that has a "special" child list (or other private
frames not in the principal child list) not known to the frame constructor
needs to deal with this explicitly.
I found that 'nsMenuFrame' that also have a 'popupList' child list have
the same code that I suggested:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/xul/base/src/nsMenuFrame.cpp&rev=1.298&root=/cvsroot&mark=322-325#315

Given what WipeContainingBlock() and other code currently does, I would say
that adding RemoveMappingsForFrameSubtree() to nsFrameList::DestroyFrames()
is probably not the right design (likely a perf problem if we do).

I think we should just add calls to RemoveMappingsForFrameSubtree()
in ::Destroy() where needed...

What do you think?

	Mats Palmgren (inactive) Assignee
	Comment 11 • 19 years ago

Correction, the first link in the last comment should be:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/base/nsCSSFrameConstructor.cpp&rev=1.1126&root=/cvsroot&mark=13173#13135

	Boris Zbarsky [:bzbarsky]
	Comment 12 • 19 years ago

Ah, I see.  Child lists that are being used for stuff that has no placeholders..

The patch looks good, then, but maybe we should have a way to enumerate such
child lists, in general?

	Mats Palmgren (inactive) Assignee
	Comment 13 • 19 years ago

Took the liberty of removing an unused variable as well

	Mats Palmgren (inactive) Assignee
	Comment 14 • 19 years ago

Checked in to trunk at 2005-09-20 13:28 PDT

-> FIXED