Last Comment Bug 696307 - "ASSERTION: Layer already in list" and crash with transform, -moz-column, abs pos, shadow
: "ASSERTION: Layer already in list" and crash with transform, -moz-column, abs...
Status: RESOLVED FIXED
[inbound]
: assertion, crash, testcase
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: x86_64 Mac OS X
: -- critical (vote)
: mozilla10
Assigned To: Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
:
Mentors:
Depends on:
Blocks: randomstyles 505115
  Show dependency treegraph
 
Reported: 2011-10-20 19:52 PDT by Jesse Ruderman
Modified: 2011-11-08 01:27 PST (History)
8 users (show)
emorley: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (crashes Firefox when loaded) (338 bytes, text/html)
2011-10-20 19:52 PDT, Jesse Ruderman
no flags Details
stack traces (7.72 KB, text/plain)
2011-10-20 19:52 PDT, Jesse Ruderman
no flags Details
2D testcase (334 bytes, text/html)
2011-10-21 15:49 PDT, Matt Woodrow (:mattwoodrow)
no flags Details
don't overwrite aChild (12.03 KB, patch)
2011-11-02 17:08 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
mats: review+
Details | Diff | Review
Don't paint pushed float twice (2.77 KB, patch)
2011-11-02 17:09 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
mats: review+
Details | Diff | Review

Description Jesse Ruderman 2011-10-20 19:52:15 PDT
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.)
Comment 1 Jesse Ruderman 2011-10-20 19:52:44 PDT
Created attachment 568592 [details]
stack traces
Comment 2 Matt Woodrow (:mattwoodrow) 2011-10-21 15:43:31 PDT
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) 2011-10-21 15:49:17 PDT
Created attachment 568797 [details]
2D testcase
Comment 4 Matt Woodrow (:mattwoodrow) 2011-10-21 16:27:54 PDT
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) (Exited; email my personal email if necessary) 2011-11-01 14:40:40 PDT
Can you get a frame tree dump to confirm that?
Comment 6 Matt Woodrow (:mattwoodrow) 2011-11-01 15:18:37 PDT
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) 2011-11-01 15:43:41 PDT
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) (Exited; email my personal email if necessary) 2011-11-01 20:45:25 PDT
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) 2011-11-02 00:51:36 PDT
The call is coming from http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsBlockFrame.cpp#6182
Comment 10 Matt Woodrow (:mattwoodrow) 2011-11-02 00:52:43 PDT
Rest of the stack beyond that is: http://pastebin.mozilla.org/1370963
Comment 11 Matt Woodrow (:mattwoodrow) 2011-11-02 00:58:04 PDT
Call stack adding NS_FRAME_IS_PUSHED_FLOAT: http://pastebin.mozilla.org/1370964
Comment 12 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-11-02 15:43:53 PDT
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) (Exited; email my personal email if necessary) 2011-11-02 16:23:11 PDT
(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) (Exited; email my personal email if necessary) 2011-11-02 16:56:03 PDT
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) (Exited; email my personal email if necessary) 2011-11-02 16:59:10 PDT
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) (Exited; email my personal email if necessary) 2011-11-02 17:08:39 PDT
Created attachment 571512 [details] [diff] [review]
don't overwrite aChild
Comment 17 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-11-02 17:09:04 PDT
Created attachment 571513 [details] [diff] [review]
Don't paint pushed float twice
Comment 18 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-11-02 17:09:49 PDT
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 (:mats) 2011-11-07 13:20:45 PST
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 (:mats) 2011-11-07 13:22:27 PST
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) (Exited; email my personal email if necessary) 2011-11-07 13:30:09 PST
(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) (Exited; email my personal email if necessary) 2011-11-07 16:21:44 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/40d852cfa7ea
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9893df2c24f

Note You need to log in before you can comment on or make changes to this bug.