117984 - [PFM]ASSERTION: frame was not removed from primary frame map before destruction...

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Johnny Stenback (:jst)]

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

Comment 1

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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

Comment 2

[Bernd]

probably a dupe of bug 101216

Comment 3

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

No, I was thinking of bug 117703.

Comment 4

[Daniel Bratell]

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

Comment 5

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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.

Comment 6

[Daniel Bratell]

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?

Comment 7

[rbs]

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

Comment 8

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

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

Comment 9

[Boris Zbarsky [:bzbarsky]]

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?

Comment 10

[Mats Palmgren (inactive)]

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?

Comment 11

[Mats Palmgren (inactive)]

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

Comment 12

[Boris Zbarsky [:bzbarsky]]

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?

Comment 13

[Mats Palmgren (inactive)]

Attachment: Final patch

Took the liberty of removing an unused variable as well

Comment 14

[Mats Palmgren (inactive)]

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

-> FIXED