366583 - "ASSERTION: NS_BLOCK_FRAME_HAS_OUTSIDE_BULLET flag set and no mBullet"

Status: VERIFIED FIXED

(Core :: Layout, defect, P2)

Description

[Jesse Ruderman]

Loading this testcase triggers:

###!!! ASSERTION: NS_BLOCK_FRAME_HAS_OUTSIDE_BULLET flag set and no mBullet: 'mBullet', file /Users/admin/trunk/mozilla/layout/generic/nsBlockFrame.h, line 324

Comment 1

[Jesse Ruderman]

Attachment: testcase

Comment 2

[Boris Zbarsky [:bzbarsky]]

This is pretty bad.  The first problem, as I see it is that we get:

#0  nsCSSFrameConstructor::ProcessPendingRestyles (this=0x87b39f8)
    at ../../../mozilla/layout/base/nsCSSFrameConstructor.cpp:12836
#1  0xb71130ce in PresShell::FlushPendingNotifications (this=0x87b3618, 
    aType=Flush_Frames) at ../../../mozilla/layout/base/nsPresShell.cpp:4792
#2  0xb72e6eda in nsBoxObject::GetFrame (this=0x881d764, aFlushLayout=0)
    at ../../../../../mozilla/layout/xul/base/src/nsBoxObject.cpp:138
#3  0xb76406d4 in nsTreeBoxObject::GetTreeBody (this=0x881d760)
    at ../../../../../../../mozilla/layout/xul/base/src/tree/src/nsTreeBoxObject.cpp:140
#4  0xb7640ce6 in nsTreeBoxObject::GetColumns (this=0x881d760, aColumns=0xbfffe7b0)
    at ../../../../../../../mozilla/layout/xul/base/src/tree/src/nsTreeBoxObject.cpp:250
#5  0xb76423ab in nsTreeColFrame::InvalidateColumns (this=0x881c0d4)
    at ../../../../../../../mozilla/layout/xul/base/src/tree/src/nsTreeColFrame.cpp:230
#6  0xb7641e1c in nsTreeColFrame::Destroy (this=0x881c0d4)
    at ../../../../../../../mozilla/layout/xul/base/src/tree/src/nsTreeColFrame.cpp:102

while destroying frames.  That's _so_ not good.  We shouldn't be processing pending restyles in the middle of frame destruction.

Then we call PresShell::GetPrimaryFrameFor from GetTreeBody, which starts walking a frame tree parts of which are in the middle of destruction, and then we get this assertion.

Of course then we go on to ask a partially-destroyed from for its nsITreeColumns, etc....

I think the right approach here is probably to InvalidateColumns off an event, but someone who knows trees should make that call.

Comment 3

[[e-mail]]

Or we could cache the nsITreeColumns in the nsTreeBoxObject.

Comment 4

[Boris Zbarsky [:bzbarsky]]

If that would prevent us from accessing the frame tree, sounds great to me.  ;)

Comment 5

[Jesse Ruderman]

> We shouldn't be processing pending restyles in the middle of frame destruction.

Should there be an assertion for this, so we can catch bugs like this earlier?

Comment 6

[Boris Zbarsky [:bzbarsky]]

Yes, there should...  Could probably just use a debug flag on the frame ctor or something where we actually remove frames.  The problem is catching all those places.

Comment 7

[Mats Palmgren (inactive)]

Attachment: fwiw

Maybe we should have some checks in PresShell::FlushPendingNotifications ?
This catches the condition in this bug. It also catches the Flush_Layout
during reflow here:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/docshell/base/nsDocShell.cpp&rev=1.820&root=/cvsroot&mark=3561#3552
other than that it's quite (for the few pages I tested).

Comment 8

[Boris Zbarsky [:bzbarsky]]

Flushing layout during reflow is ok -- the IsSafeToFlush check will just ignore the call.

So I think we should only do these asserts if isSafeToFlush returns true...

Comment 9

[Jesse Ruderman]

This also triggers:

###!!! ASSERTION: frame was not removed from primary frame map before destruction or was readded to map after being removed: 'Not Reached', file /Users/jruderman/trunk/mozilla/layout/base/nsFrameManager.cpp, line 708

Comment 10

[Jesse Ruderman]

Now I get:

###!!! ASSERTION: Null out-of-flow for placeholder?: 'outOfFlow', file /Users/jruderman/trunk/mozilla/layout/base/../generic/nsPlaceholderFrame.h, line 175

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think this is now basically bug 391178 (and its brother bug 399715).

Reassigning to dholbert on those grounds. Hope that's OK.

Comment 12

[Daniel Holbert [:dholbert]]

Fixed by checkin of bug 391178.

Comment 13

[Jesse Ruderman]

Crashtest checked in.

Comment 14

[Carsten Book [:Tomcat]]

verified fixed using the testcase -> no assertion -> verified fixed