500063 - "ASSERTION: Already have an undisplayed context entry for aContent" while running leak tests

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Log at:

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1245784868.1245789314.24977.gz

This is also listing a lot of leaks, but that's due to the assertion being fatal and causing us to shut down.

Comment 1

[(not currently active) Ted Mielczarek]

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1246988269.1246989772.15906.gz
WINNT 5.2 mozilla-central leak test build on 2009/07/07 10:37:49

Comment 2

[Timothy Nikkel (:tnikkel)]

After several thousand runs of the leaktest I think I know what is going on here; the stackwalker in nsStackWalk.cpp fails in my winxp vm for some reason, so I can't be 100% sure.

The duplicate undisplayed entry is the hidden input inside the fieldset on http://www.mozilla.org/newlayout/samples/test8.html. It's a good bet that the three "recurring into frame construction" warnings are caused by a RecreateFramesForContent on the fieldset when the legend is inserted. Most of the time the insert on the legend is not needed because the legend is already available when the fieldset frame is created. Hence why this assert is so rare. The RecreateFramesForContent call eventually does a ProcessChildren on the fieldset, and that puts the hidden input into the undisplayed map. But we are still in the nsCSSFrameConstructor::ContentAppended loop at around line 6260 where we call ContentInserted for each appended child. This loop skips over content that already has a frame exactly because of a situation like this, but we still call ContentInserted on content that's already in the undisplayed map.

Would adding a check if the content is already in the undisplayed map to this loop be the right thing to do? Or would that cause problems when a "display: none" style is removed from a node and we need to remove the undisplayed entry and create a frame?

Comment 3

[Boris Zbarsky [:bzbarsky]]

> Would adding a check if the content is already in the undisplayed map to this
> loop be the right thing to do?

In the loop in ContentAppended?  Absolutely.

Comment 4

[Timothy Nikkel (:tnikkel)]

Attachment: patch

Check if there is already an undisplayed entry as well as if there is already a frame.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/92449f4c0f3e