696307 - "ASSERTION: Layer already in list" and crash with transform, -moz-column, abs pos, shadow

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when loaded)

###!!! ASSERTION: Layer already in list???: '!mNewChildLayers.Contains(ownLayer)', file layout/base/FrameLayerBuilder.cpp, line 1415

Crash [@ mozilla::::ContainerState::Finish]
(Opt: bp-88ca001a-44a4-43be-84b9-836382111020)

(Similar symptoms to bug 691367, but I can't reproduce that bug.)

Comment 1

[Jesse Ruderman]

Attachment: stack traces

Comment 2

[Matt Woodrow (:mattwoodrow)]

Looks like this isn't a 3d transforms issue as such.

I can reproduce the problem, by replacing -moz-transform-style: preserve-3d with -moz-transform: translatex(0px). It doesn't crash however because of the layers not being active.

The relevant part of the display list is:

WrapList(0x6e17d3c) ColumnSet(div)(1)(0x138c1b28) (480,4380,56580,1153)(0,0,0,0) key=56
  nsDisplayTransform(0x6e17c90) ColumnSet(div)(1)(0x138c1b28) (480,4380,56580,1153)(0,0,0,0) key=54
    ColumnRule(0x1385766c) ColumnSet(div)(1)(0x138c1b28) (480,4380,56580,1152)(0,0,0,0) key=18
      WrapList(0x13857860) Block(div)(1)(0x6ae6058) (0,0,0,0)(0,0,0,0) key=56
      WrapList(0x6e17c50) Block(div)(1)(0x6ae6058) (0,0,0,0)(0,0,0,0) key=56
      Text(0x1385769c) Text(0)(0x6ae6010) (37127,4476,813,960)(0,0,0,0) key=50
      WrapList(0x13857820) Block(div)(0)(0x6ae6330) (18700,4380,480,480)(0,0,0,0) key=56
        nsDisplayTransform(0x13857774) Block(div)(0)(0x6ae6330) (18700,4380,480,480)(0,0,0,0) key=54
          BoxShadowOuter(0x138576cc) Block(div)(0)(0x13bdded0) (18700,4380,480,480)(0,0,0,0) key=4
          BoxShadowInner(0x13857720) Block(div)(0)(0x13bdded0) (18700,4380,0,0)(0,0,0,0) key=5
      WrapList(0x6e17c10) Block(div)(0)(0x6ae6330) (18700,4380,480,480)(0,0,0,0) key=56
        nsDisplayTransform(0x13857948) Block(div)(0)(0x6ae6330) (18700,4380,480,480)(0,0,0,0) key=54
          BoxShadowOuter(0x138578a0) Block(div)(0)(0x13bdded0) (18700,4380,480,480)(0,0,0,0) key=4
          BoxShadowInner(0x138578f4) Block(div)(0)(0x13bdded0) (18700,4380,0,0)(0,0,0,0) key=5

Note the two nsDisplayTransforms with the same frame.

Roc: Any ideas on what could cause this?

Comment 3

[Matt Woodrow (:mattwoodrow)]

Attachment: 2D testcase

Comment 4

[Matt Woodrow (:mattwoodrow)]

I don't understand this code well enough to get far.

I've got stack traces for the creation of both BoxShadowInner's.

http://pastebin.mozilla.org/1361507

Looks like two different frames (0x1310f690 and 0x1310e6c8) are sharing the same child (0x1310ec10)

Comment 5

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

Can you get a frame tree dump to confirm that?

Comment 6

[Matt Woodrow (:mattwoodrow)]

Frame tree dump is: http://pastebin.mozilla.org/1370711

Similar to the above stack trace we are getting two calls to BuildDisplayListForChild with the same child

nsIFrame::BuildDisplayListForChild (this=0x1093d1258, aBuilder=0x7fff5fbf7f20, aChild=0x10921c918, aDirtyRect=@0x7fff5fbec038, aLists=@0x7fff5fbeb4a8, aFlags=4)

nsIFrame::BuildDisplayListForChild (this=0x10921da60, aBuilder=0x7fff5fbf7f20, aChild=0x10921c918, aDirtyRect=@0x7fff5fbec038, aLists=@0x7fff5fbec498, aFlags=0)

Comment 7

[Matt Woodrow (:mattwoodrow)]

Sorry, to clarify, the first call to BuildDisplayListForChild was actually with aChild=0x10921c9b8 (The Placeholder frame) and http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#1885 is modifying the parameter to aChild=0x10921c918.

Modifying parameters definitely make debugging more fun :)

Comment 8

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

What's the stack for 0x10921da60 calling BuildDisplayListForChild directly on 0x10921c918? It shouldn't be doing that, floats should only be painted through their placeholders.

Comment 9

[Matt Woodrow (:mattwoodrow)]

The call is coming from http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsBlockFrame.cpp#6182

Comment 10

[Matt Woodrow (:mattwoodrow)]

Rest of the stack beyond that is: http://pastebin.mozilla.org/1370963

Comment 11

[Matt Woodrow (:mattwoodrow)]

Call stack adding NS_FRAME_IS_PUSHED_FLOAT: http://pastebin.mozilla.org/1370964

Comment 12

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

OK, I understand now, thanks.

So this would be a situation where the float's placeholder is in the first column but the float itself got pushed to the next column, and the first column's block paints the float through the placeholder and the second column's block paints the float directly. We should be able to write a reftest for that. In this case we should paint the float through the second column, not the first, since any clipping or other effects that apply to the second column are the ones that should be applied.

Comment 13

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

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #12)
> We should be able to write a
> reftest for that. In this case we should paint the float through the second
> column, not the first, since any clipping or other effects that apply to the
> second column are the ones that should be applied.

Hmm, or should we? :-(

A float inside a <span style="opacity:0.5"> should be painted inside that opacity group, even if the span ends before next column.

Comment 14

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

The problem is that if we add any features that let individual columns or similar (regions) be styled differently (which are definitely in the works), we have a conflict between a stacking context covering content in different columns, and a stacking context for a single column.

Comment 15

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

nsContainerFrame::DisplayOverflowContainers has a similar problem. Content that should be in a single stacking context can break into parts, some of which end up in overflow containers, and we'll paint them as separate stacking contexts.

Comment 16

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

Attachment: don't overwrite aChild

Comment 17

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

Attachment: Don't paint pushed float twice

Comment 18

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

I'm not sure what the long-term solution is for comment #14 --- it depends on the way specs evolve --- but this seems like a good fix for now.

Comment 19

[Mats Palmgren (inactive)]

Comment on attachment 571513 [details] [diff] [review]
Don't paint pushed float twice

"if it's painted by a block ... it will be painted by that block" sounds
a bit peculiar to me.  Would something like "If 'child' is a pushed float
then it's owned by a block that's not an ancestor of the placeholder
and it will be painted by that block ..." be better?

Comment 20

[Mats Palmgren (inactive)]

Just to be clear, the last comment was referring to the added comment:
+ // If 'child' is a float which is painted by a block that's not an
+ // ancestor of the placeholder, it will be painted by that block and should
+ // not be painted through the placeholder.

Comment 21

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

(In reply to Mats Palmgren [:mats] from comment #19)
> "if it's painted by a block ... it will be painted by that block" sounds
> a bit peculiar to me.  Would something like "If 'child' is a pushed float
> then it's owned by a block that's not an ancestor of the placeholder
> and it will be painted by that block ..." be better?

Yes, sounds good

Comment 22

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/40d852cfa7ea
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9893df2c24f

Comment 23

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/40d852cfa7ea
https://hg.mozilla.org/mozilla-central/rev/b9893df2c24f