Closed Bug 117984 Opened 18 years ago Closed 14 years ago

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

Categories

(Core :: Layout, defect)

x86
All
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: jst, Assigned: mats)

References

()

Details

Attachments

(2 files)

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...
There is another bug on seeing that with object frames that get replaced.
probably a dupe of bug 101216
The assertions are kind of annoying. Is there a simple fix for whatever bug this 
is?
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.
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?
Another page where the assertion is a always firing: 
http://news.bbc.co.uk/sport3/worldcup2002/hi/history/newsid_1966000/1966379.stm
Summary: ASSERTION: frame was not removed from primary frame map before destruction... → [PFM]ASSERTION: frame was not removed from primary frame map before destruction...
Assignee: dbaron → mats.palmgren
OS: Windows 2000 → All
Attached patch Patch rev. 1Splinter Review
Call 'RemoveMappingsForFrameSubtree()' before destroying 'mPopupFrames'.
Attachment #194823 - Flags: superreview?(bzbarsky)
Attachment #194823 - Flags: review?(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?
Attachment #194823 - Flags: superreview?(bzbarsky)
Attachment #194823 - Flags: superreview+
Attachment #194823 - Flags: review?(bzbarsky)
Attachment #194823 - Flags: review+
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?
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?
Attached patch Final patchSplinter Review
Took the liberty of removing an unused variable as well
Checked in to trunk at 2005-09-20 13:28 PDT

-> FIXED
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Depends on: 310505
You need to log in before you can comment on or make changes to this bug.