Last Comment Bug 682922 - Crash with -moz-transform-style: preserve-3d
: Crash with -moz-transform-style: preserve-3d
Status: VERIFIED FIXED
[inbound][qa!]
: assertion, crash, testcase, verified-aurora, verified-beta
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: x86_64 All
: -- critical (vote)
: mozilla9
Assigned To: Matt Woodrow (:mattwoodrow) (PTO until 27 June)
:
Mentors:
Depends on:
Blocks: randomstyles 505115
  Show dependency treegraph
 
Reported: 2011-08-29 11:48 PDT by Jesse Ruderman
Modified: 2011-12-01 14:36 PST (History)
9 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-


Attachments
testcase (crashes Firefox when loaded) (120 bytes, text/html)
2011-08-29 11:48 PDT, Jesse Ruderman
no flags Details
stack traces (16.33 KB, text/plain)
2011-08-29 11:49 PDT, Jesse Ruderman
no flags Details
Use the parent frame if the child doesn't have one (3.51 KB, patch)
2011-08-30 19:38 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
no flags Details | Diff | Review
Always create nsDisplayClips with a frame if we're preserving 3d (11.47 KB, patch)
2011-09-01 23:00 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
no flags Details | Diff | Review
Block preserve-3d only scroll frames. (7.51 KB, patch)
2011-09-03 20:25 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
roc: review+
Details | Diff | Review

Description Jesse Ruderman 2011-08-29 11:48:50 PDT
Created attachment 556627 [details]
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 2011-08-29 11:49:26 PDT
Created attachment 556628 [details]
stack traces
Comment 2 Jesse Ruderman 2011-08-29 11:52:18 PDT
bp-c2288e3b-716c-4192-b554-86a192110829
Comment 3 Marcia Knous [:marcia - use ni] 2011-08-29 12:17:00 PDT
Real world site that crashes in this stack: plexapp.com
Comment 4 Marcia Knous [:marcia - use ni] 2011-08-29 16:27:26 PDT
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-7 (review requests must explain patch) 2011-08-29 17:57:28 PDT
Are these happening only when people enable the 3D transforms pref, or otherwise?
Comment 6 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2011-08-30 19:38:02 PDT
Created attachment 557077 [details] [diff] [review]
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) (Exited; email my personal email if necessary) 2011-08-30 19:56:16 PDT
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) (PTO until 27 June) 2011-09-01 23:00:24 PDT
Created attachment 557771 [details] [diff] [review]
Always create nsDisplayClips with a frame if we're preserving 3d
Comment 9 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-09-02 04:19:07 PDT
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) (Exited; email my personal email if necessary) 2011-09-02 04:24:29 PDT
Oh, I think nsIFrame::Preserves3DChildren needs to check for clipping and filters instead of nsIFrame::Preserves3D!
Comment 11 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2011-09-02 13:17:10 PDT
(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) (PTO until 27 June) 2011-09-02 13:17:37 PDT
(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) (PTO until 27 June) 2011-09-02 14:49:18 PDT
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) (Exited; email my personal email if necessary) 2011-09-03 05:58:29 PDT
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) (Exited; email my personal email if necessary) 2011-09-03 06:40:14 PDT
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) (PTO until 27 June) 2011-09-03 20:25:18 PDT
Created attachment 558115 [details] [diff] [review]
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) (PTO until 27 June) 2011-09-04 15:26:57 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/710f3b41a5d7
Comment 18 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-09-05 05:05:53 PDT
http://hg.mozilla.org/mozilla-central/rev/710f3b41a5d7
Comment 19 Ioana (away) 2011-11-17 05:18:39 PST
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, jst@mozilla.com) 2011-12-01 14:36:56 PST
This was fixed in 9, but not tracking worthy at this point.

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