Last Comment Bug 691106 - 3D transforms Box1 demo crashes
: 3D transforms Box1 demo crashes
Status: RESOLVED FIXED
[inbound]
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla10
Assigned To: Nobody; OK to take it and work on it
:
: Milan Sreckovic [:milan]
Mentors:
http://romaxa.bolshe.net/css3d/box1/b...
Depends on:
Blocks: 505115 690965
  Show dependency treegraph
 
Reported: 2011-10-01 17:09 PDT by Matt Woodrow (:mattwoodrow)
Modified: 2011-10-03 08:04 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Merge sequential nsDisplayTransforms (2.98 KB, patch)
2011-10-01 21:42 PDT, Matt Woodrow (:mattwoodrow)
no flags Details | Diff | Splinter Review
Merge sequential nsDisplayTransforms and give them unique keys (8.28 KB, patch)
2011-10-02 19:35 PDT, Matt Woodrow (:mattwoodrow)
no flags Details | Diff | Splinter Review
Merge sequential nsDisplayTransforms and give them unique keys v2 (8.40 KB, patch)
2011-10-02 20:37 PDT, Matt Woodrow (:mattwoodrow)
roc: review+
Details | Diff | Splinter Review

Description Matt Woodrow (:mattwoodrow) 2011-10-01 17:09:21 PDT
https://crash-stats.mozilla.com/report/index/bp-2a2a8e08-6159-445b-bc76-2f8fa2111001

       WrapList 0x661b290(Block(div)(1)) (34200,8928,18000,12000)(0,0,0,0)
                        WrapList 0x661b290(Block(div)(1)) (34200,8928,18000,12000)(0,0,0,0)
                            WrapList 0x661bfc8(Block(figure)(1)) (34200,8928,18000,12000)(0,0,0,0)
                                nsDisplayTransform 0x661bfc8(Block(figure)(1)) (34200,8928,18000,12000)(0,0,0,0)
                                    Background 0x661bfc8(Block(figure)(1)) (34200,8928,18000,12000)(0,0,0,0) uniform
                                nsDisplayTransform 0x661bfc8(Block(figure)(1)) (34200,8928,18000,12000)(0,0,0,0)
                                    Border 0x661bfc8(Block(figure)(1)) (34200,8928,18000,12000)(0,0,0,0)
                                nsDisplayTransform 0x661c1d0(Text(0)) (41639,11688,3122,6480)(0,0,0,0)
                                    Text 0x661c1d0(Text(0)) (41639,11688,3122,6480)(0,0,0,0)

We create this display list, which has multiple nsDisplayTransforms with the same frame pointer. 
FrameLayerBuilder then throws assertions:
###!!! ASSERTION: Layer already in list???: '!mNewChildLayers.Contains(ownLayer)', file /Users/mattwoodrow/mozilla-central2/layout/base/FrameLayerBuilder.cpp, line 1447
and crashes.

Previously we were assuming that the child list (when we wrap each member in an nsDisplayTransform) had each item from a different frame. This clearly isn't true in this case as both the Background and Border are the same frame.

We could group sequential items with the same frame under a single nsDisplayTransform, not sure if this would fix all the possible cases, or just this one.
Comment 1 Matt Woodrow (:mattwoodrow) 2011-10-01 17:40:04 PDT
I think the exact question is is it possible to have a display list structure like this:

 - Background(frame1)
 - nsDisplayTransform(frame2)
 - Border(frame1)

Where there are multiple display items with the same frame, separated by an nsDisplayTransform.

If we can't get this, and display items with the same frame are always sequential (and transformed frames are always entired contained within an nsDisplayTransform), then we can coalesce these under a single nsDisplayTransform.
Comment 2 Matt Woodrow (:mattwoodrow) 2011-10-01 21:42:32 PDT
Created attachment 564016 [details] [diff] [review]
Merge sequential nsDisplayTransforms

Implementation of my suggestion, fixes this demo.

Still not sure if it's the correct fix for all cases though.
Comment 3 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-10-02 01:33:05 PDT
(In reply to Matt Woodrow (:mattwoodrow) from comment #1)
> I think the exact question is is it possible to have a display list
> structure like this:
> 
>  - Background(frame1)
>  - nsDisplayTransform(frame2)
>  - Border(frame1)
> 
> Where there are multiple display items with the same frame, separated by an
> nsDisplayTransform.
> 
> If we can't get this, and display items with the same frame are always
> sequential (and transformed frames are always entired contained within an
> nsDisplayTransform), then we can coalesce these under a single
> nsDisplayTransform.

That's a good idea anyway since it reduces the number of display items and the number of layers created.

Unfortunately the invariant you need for this to solve all the problems isn't valid. For example a block could have a background and an outline with some transformed content between them.
Comment 4 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-10-02 01:35:18 PDT
Actually we can do better than this patch. We can accumulate any run of consecutive items that are going to be wrapped in a new nsDisplayTransform.
Comment 5 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-10-02 01:47:27 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #4)
> Actually we can do better than this patch. We can accumulate any run of
> consecutive items that are going to be wrapped in a new nsDisplayTransform.

Hmm, I guess we can't easily do that because we wouldn't be able to associate such lists with a particular frame.

This constraint that we can have only one display item of a given type for a given frame is problematic. However FrameLayerBuilder isn't supposed to depend on that constraint; it should really only depend on the constraint that there's one display item using a given GetPerFrameKey for a given frame. Right now GetPerFrameKey just returns the type but we can change that. So one way to solve the underlying problem here would be to override nsDisplayTransform::GetPerFrameKey to return a different value for each nsDisplayTransform we need to associate with a frame, allocate a range of keys for nsDisplayTransforms and use a new key for each nsDisplayTransform we create for a given frame. If we did this, we could associate the wrapper nsDisplayTransforms with the transformed frame, which would let us wrap lots of child items at a time.
Comment 6 Matt Woodrow (:mattwoodrow) 2011-10-02 19:35:16 PDT
Created attachment 564106 [details] [diff] [review]
Merge sequential nsDisplayTransforms and give them unique keys

This adds an index parameter to nsDisplayTransform which is factored into the GetPerFrameKey calculation.

We then create nsDisplayTransforms around as many child objects as possible, and use the transformed frame pointer instead of the child frame pointer.
Comment 7 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-10-02 19:58:24 PDT
Comment on attachment 564106 [details] [diff] [review]
Merge sequential nsDisplayTransforms and give them unique keys

Review of attachment 564106 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsDisplayList.h
@@ +2125,5 @@
>                                     nsRegion *aVisibleRegion,
>                                     const nsRect& aAllowVisibleRegionExpansion);
>    virtual bool TryMerge(nsDisplayListBuilder *aBuilder, nsDisplayItem *aItem);
> +  
> +  virtual PRUint32 GetPerFrameKey() { return (mIndex << 16) | nsDisplayItem::GetPerFrameKey(); }

Allow 32-bit indices and shift left by nsDisplayItem::TYPE_BITS (8).

Define INDEX_MAX here to PR_UINT32_MAX >> TYPE_BITS.

::: layout/generic/nsFrame.cpp
@@ +1522,5 @@
>  {
>    nsresult rv = NS_OK;
>    nsDisplayList newList;
> +  nsDisplayList temp;
> +  bool flushed = true;

Don't need "flushed", just use temp.IsEmpty().

@@ +1577,5 @@
>        return rv;
> +  }
> +    
> +  if (!flushed) {
> +    newList.AppendToTop(new (aBuilder) nsDisplayTransform(aBuilder, aFrame, &temp, aIndex++));

We need to do something to detect aIndex overflows since people will be able to hit it with malicious content. You'd have 2^24 nsDisplayTransforms at that point so I don't think we have to worry about it for real content. One option would be to just not transform the remaining content. Another option would be to just exit the loop, effectively making the remaining content disappear.
Comment 8 Matt Woodrow (:mattwoodrow) 2011-10-02 20:37:34 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #7)
> We need to do something to detect aIndex overflows since people will be able
> to hit it with malicious content. You'd have 2^24 nsDisplayTransforms at
> that point so I don't think we have to worry about it for real content. One
> option would be to just not transform the remaining content. Another option
> would be to just exit the loop, effectively making the remaining content
> disappear.

Good point, I didn't even consider overflow.
Comment 9 Matt Woodrow (:mattwoodrow) 2011-10-02 20:37:58 PDT
Created attachment 564112 [details] [diff] [review]
Merge sequential nsDisplayTransforms and give them unique keys v2
Comment 10 Matt Woodrow (:mattwoodrow) 2011-10-02 21:05:22 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/f78254d32632
Comment 11 Marco Bonardo [::mak] 2011-10-03 08:04:29 PDT
https://hg.mozilla.org/mozilla-central/rev/f78254d32632

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