682922 - Crash with -moz-transform-style: preserve-3d

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when loaded)

###!!! ASSERTION: No frame?: 'mFrame', file layout/base/nsDisplayList.h, line 817

###!!! ASSERTION: Can't take the transform based on a null frame!: 'aFrame', file layout/base/nsDisplayList.cpp, line 2792

###!!! ASSERTION: Cannot get transform matrix for a null frame!: 'aFrame', file layout/base/nsDisplayList.cpp, line 2448

Crash [@ nsDisplayTransform::GetResultingTransformMatrix]

Comment 1

[Jesse Ruderman]

Attachment: stack traces

Comment 2

[Jesse Ruderman]

bp-c2288e3b-716c-4192-b554-86a192110829

Comment 3

[Marcia Knous [:marcia]]

Real world site that crashes in this stack: plexapp.com

Comment 4

[Marcia Knous [:marcia]]

Adding the Windows specific signature that I came across in crash stats today - https://crash-stats.mozilla.com/report/index/0b5caa2a-6e26-430c-86cf-2682a2110829.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Are these happening only when people enable the 3D transforms pref, or otherwise?

Comment 6

[Matt Woodrow (:mattwoodrow)]

Attachment: Use the parent frame if the child doesn't have one

Not 100% sure if my logic is correct for all cases here. It at least fixes the reported crash.

Comment 7

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

Comment on attachment 557077 [details] [diff] [review]
Use the parent frame if the child doesn't have one

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

This is going to lead to multiple display items with the same frame and same type, which we don't want in general (except for nsDisplayClip, where it's OK). (That prohibition needs to be documented somewhere, maybe nsDisplayList.h.)

Probably the thing to do here is to require there be a non-null GetUnderlyingFrame (if there isn't, assert and bail out without wrapping). The two callers that pass a null underlying frame to nsDisplayClip are nsOverflowClipWrapper::WrapList and nsAbsPosClipWrapper::WrapList. For those cases we can check if the container frame is going to wrap its descendants in an nsDisplayTransform; if it will, we can wrap the items individually with their underlying frames. The signature of WrapList will need to be changed so that it takes an output list parameter to which multiple items can be appended.

The long-term solution here is to get rid of nsDisplayClip entirely and store clip rects/rounded rects directly on other display items. Then all display items can have an underlying frame.

Comment 8

[Matt Woodrow (:mattwoodrow)]

Attachment: Always create nsDisplayClips with a frame if we're preserving 3d

Comment 9

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

If we have a scrollframe with preserve-3d, plus some grandchildren with their own transforms, then even with your latest patch we're going to end up assigning two nsDisplayTransforms to each of those grandchildren.

The thing is, if we have an overflow:non-visible element with preserve-3d, the preserve-3d is never relevant, right? All the descendants of the element that could possibly have their own 3D transforms are clipped. So couldn't we just ignore preserve-3d on scrollframes, and force the transform on a scrollframe to be applied to the entire frame?

Then again, nsIFrame::Preserves3D() already checks for overflow and abs-pos clipping on the frame. So what's actually happening in this testcase? Are we just getting burned by an edge case because the frame has preserve-3d but no actual transform? Do we need an extra IsTransformed() check somewhere?

Comment 10

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

Oh, I think nsIFrame::Preserves3DChildren needs to check for clipping and filters instead of nsIFrame::Preserves3D!

Comment 11

[Matt Woodrow (:mattwoodrow)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #9)
> If we have a scrollframe with preserve-3d, plus some grandchildren with
> their own transforms, then even with your latest patch we're going to end up
> assigning two nsDisplayTransforms to each of those grandchildren.
> 
> The thing is, if we have an overflow:non-visible element with preserve-3d,
> the preserve-3d is never relevant, right? All the descendants of the element
> that could possibly have their own 3D transforms are clipped. So couldn't we
> just ignore preserve-3d on scrollframes, and force the transform on a
> scrollframe to be applied to the entire frame?
> 
> Then again, nsIFrame::Preserves3D() already checks for overflow and abs-pos
> clipping on the frame. So what's actually happening in this testcase? Are we
> just getting burned by an edge case because the frame has preserve-3d but no
> actual transform? Do we need an extra IsTransformed() check somewhere?

The frame with preserve-3d (nsGfxScrollFrame) isn't actually clipped. It's a child nsBlockFrame that has the clip, and a grandchild (nsTextFrame?) that we are trying to wrap an nsDisplayTransform around.

Maybe Preserves3DChildren() needs to iterate all of its children and check for any that have a transform. I don't think that actually helps us where we have a child with a transform, as well as a child with a clip though.

Comment 12

[Matt Woodrow (:mattwoodrow)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #10)
> Oh, I think nsIFrame::Preserves3DChildren needs to check for clipping and
> filters instead of nsIFrame::Preserves3D!

This patch does that! :)

Comment 13

[Matt Woodrow (:mattwoodrow)]

So the basic problem case we have is a frame structure similar to this:

nsGfxScrollFrame - preserve-3d
    nsBlockFrame - clipped
        nsTextFrame - transformed

Which is turned into:

nsDisplayWrapList(nsGfxScrollFrame)
    nsDisplayTransform(nsTextFrame)
        nsDisplayClip(nsTextFrame)
            nsDisplayTransform(nsTextFrame)
                nsDisplayText(nsTextFrame)

It seems to me that the fundamental problem here is that the clips belong to the nsBlockFrame, yet are created for the nsTextFrame. The nsBlockFrame is instead entirely omitted from the display list. If the clip was created for the block frame, it's parent nsDisplayTransform would be too and things (at least from the nsDisplayTransform perspective) would work correctly. As it currently stands both nsDisplayTransforms will read the transform set on the nsTextFrame, none from the nsGfxScrollFrame.

I assume there is a reason for the nsBlockFrame to disappear though, can it be worked around?

Comment 14

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

We could put the nsDisplayClip(s) on the nsBlockFrame, yeah. It wouldn't be that hard, we'd just pass an extra parameter through OverflowClip and friends telling it to use the scrollable frame as the underlying frame for the clip it creates. However, that wouldn't solve this problem because we could end up creating multiple nsDisplayClips for the same scrollable nsBlockFrame, and then you'd create multiple nsDisplayTransform wrappers for the same frame, and we lose.

It's not possible to ensure that there's always one nsDisplayClip for the scrolled frame. The problem is that in CSS, you can have arbitrary interleaving of scrolled content with non-scrolled content in z-order, and the non-scrolled content mustn't be clipped. E.g.
  <div style="overflow:scroll">
    <div style="position:relative; z-index:0">Hello</div>
    <div style="position:absolute; z-index:1">Kitty</div>
    <div style="position:relative; z-index:2">Kitty2</div>
  </div>
"Kitty" does not get scrolled and is not clipped. So "Hello" and "Kitty2" need independent nsDisplayClips.

(In reply to Matt Woodrow (:mattwoodrow) from comment #12)
> This patch does that! :)

Indeed! So, with that fixed, why are we still trying to call WrapPreserve3DList in Jesse's testcase? Preserves3DChildren should be returning false everywhere.

Comment 15

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

Urgh, I really should read everything...

(In reply to Matt Woodrow (:mattwoodrow) from comment #11)
> The frame with preserve-3d (nsGfxScrollFrame) isn't actually clipped. It's a
> child nsBlockFrame that has the clip, and a grandchild (nsTextFrame?) that
> we are trying to wrap an nsDisplayTransform around.

Note that ApplyOverflowClipping doesn't capture all clipping due to the "overflow" property. Like the comment says:
  // Only -moz-hidden-unscrollable is handled here (and 'hidden' for table
  // frames, and any non-visible value for blocks in a paginated context).
  // Other overflow clipping is applied by nsHTML/XULScrollFrame.

I think we need to explicitly check for scrollframes and make them return false from nsIFrame::Preserves3DChildren. The question is whether that is correct behavior. I think it is, although the reasoning is subtle:
* The question is whether can there be any descendant content to which we should propagate the scrollframe's 3D transform
* Because the scrollframe has a transform, it establishes the containing block for abs-pos and fixed-pos descendants, and that ensures that those elements are scrolled by the scrollframe, and clipped by it too
* Of course, the same applies to non-positioned descendants too
* So all descendant content in the DOM is clipped by the scrollframe, and therefore should not participate in preserve-3d
* The scrollbar parts are not clipped by the scrollframe, but they won't have 3D transforms of their own, so it doesn't matter if they don't participate in preserve-3d

One wrinkle is that point 2 is actually treated differently by Webkit vs other browsers. In Webkit, a transformed scrollable element establishes a containing block for positioned descendants, but those descendants are not scrolled or clipped. In other browsers, a transformed scrollable element establishes a containing block for positioned descendants, and those descendants are scrolled and clipped. (In all browsers, a positioned scrollable element establishes a containing block for positioned descendants, and those descendants are scrolled and clipped.) I think we can ignore this issue.

Comment 16

[Matt Woodrow (:mattwoodrow)]

Attachment: Block preserve-3d only scroll frames.

Thanks roc, I've implented blocking of preserve-3d when we have a scroll frame.

Comment 17

[Matt Woodrow (:mattwoodrow)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/710f3b41a5d7

Comment 18

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

http://hg.mozilla.org/mozilla-central/rev/710f3b41a5d7

Comment 19

[Ioana (away)]

Verified as fixed on:
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:9.0) Gecko/20100101 Firefox/9.0
Mozilla/5.0 (X11; Linux x86_64; rv:9.0) Gecko/20100101 Firefox/9.0
Mozilla/5.0 (Windows NT 5.1; rv:9.0) Gecko/20100101 Firefox/9.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:9.0) Gecko/20100101 Firefox/9.0

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:10.0a2) Gecko/20111116 Firefox/10.0a2
Mozilla/5.0 (X11; Linux x86_64; rv:10.0a2) Gecko/20111116 Firefox/10.0a2
Mozilla/5.0 (Windows NT 5.1; rv:10.0a2) Gecko/20111110 Firefox/10.0a2
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:10.0a2) Gecko/20111114 Firefox/10.0a2

Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:11.0a1) Gecko/20111116 Firefox/11.0a1
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:11.0a1) Gecko/20111115 Firefox/11.0a1
Mozilla/5.0 (Windows NT 5.1; rv:11.0a1) Gecko/20111110 Firefox/11.0a1
Mozilla/5.0 (X11; Linux x86_64; rv:11.0a1) Gecko/20111117 Firefox/11.0a1

The crash doesn't reproduce anymore when opening the test case or plexapp.com.

Comment 20

[Johnny Stenback (:jst)]

This was fixed in 9, but not tracking worthy at this point.