The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla10

Status

()

Core
Layout
--
critical
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Jesse Ruderman, Assigned: roc)

Tracking

(Blocks: 1 bug, {assertion, crash, testcase})

Trunk
mozilla10
x86_64
Mac OS X
assertion, crash, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [inbound], crash signature)

Attachments

(5 attachments)

(Reporter)

Description

6 years ago
Created attachment 568591 [details]
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.)
(Reporter)

Comment 1

6 years ago
Created attachment 568592 [details]
stack traces
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?
Created attachment 568797 [details]
2D testcase
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)
Can you get a frame tree dump to confirm that?
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)
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 :)
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.
The call is coming from http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsBlockFrame.cpp#6182
Rest of the stack beyond that is: http://pastebin.mozilla.org/1370963
Call stack adding NS_FRAME_IS_PUSHED_FLOAT: http://pastebin.mozilla.org/1370964
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.
(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.
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.
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.
Created attachment 571512 [details] [diff] [review]
don't overwrite aChild
Assignee: nobody → roc
Attachment #571512 - Flags: review?(matspal)
Created attachment 571513 [details] [diff] [review]
Don't paint pushed float twice
Attachment #571513 - Flags: review?(matspal)
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.

Updated

6 years ago
Attachment #571512 - Flags: review?(matspal) → review+
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?
Attachment #571513 - Flags: review?(matspal) → review+
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.
(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
https://hg.mozilla.org/integration/mozilla-inbound/rev/40d852cfa7ea
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9893df2c24f
Whiteboard: [inbound]
https://hg.mozilla.org/mozilla-central/rev/40d852cfa7ea
https://hg.mozilla.org/mozilla-central/rev/b9893df2c24f
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
You need to log in before you can comment on or make changes to this bug.