Bug 230079 – Some css-transforms tests assert in debug

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Simon Fraser (smfr)]

These tests:

imported/w3c/web-platform-tests/css/css-transforms/transform-transformed-tbody-contains-fixed-position.html
imported/w3c/web-platform-tests/css/css-transforms/transform-transformed-tfoot-contains-fixed-position.html
imported/w3c/web-platform-tests/css/css-transforms/transform-transformed-thead-contains-fixed-position.html
imported/w3c/web-platform-tests/css/css-transforms/transform-transformed-tr-contains-fixed-position.html
imported/w3c/web-platform-tests/css/css-transforms/transform-transformed-tr-percent-height-child.html

hit an assertion:

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.JavaScriptCore      	0x00000003da6a4a9e WTFCrash + 14 (Assertions.cpp:321)
1   com.apple.WebCore             	0x00000003bb25c96b WTFCrashWithInfo(int, char const*, char const*, int) + 27 (Assertions.h:703)
2   com.apple.WebCore             	0x00000003bfa2b4c6 WebCore::RenderObject::offsetFromAncestorContainer(WebCore::RenderElement&) const + 262 (RenderObject.cpp:1446)
3   com.apple.WebCore             	0x00000003bf897dae WebCore::RenderBox::pushMappingToContainer(WebCore::RenderLayerModelObject const*, WebCore::RenderGeometryMap&) const + 238 (RenderBox.cpp:2291)
4   com.apple.WebCore             	0x00000003bf93bfab WebCore::RenderGeometryMap::pushMappingsToAncestor(WebCore::RenderObject const*, WebCore::RenderLayerModelObject const*) + 91 (RenderGeometryMap.cpp:140)
5   com.apple.WebCore             	0x00000003bf93c3a6 WebCore::RenderGeometryMap::pushMappingsToAncestor(WebCore::RenderLayer const*, WebCore::RenderLayer const*, bool) + 598 (RenderGeometryMap.cpp:197)
6   com.apple.WebCore             	0x00000003bf971313 WebCore::RenderLayer::updateLayerPositions(WebCore::RenderGeometryMap*, WTF::OptionSet<WebCore::RenderLayer::UpdateLayerPositionsFlag>) + 179 (RenderLayer.cpp:941)
7   com.apple.WebCore             	0x00000003bf971b28 WebCore::RenderLayer::updateLayerPositions(WebCore::RenderGeometryMap*, WTF::OptionSet<WebCore::RenderLayer::UpdateLayerPositionsFlag>) + 2248 (RenderLayer.cpp:1032)
8   com.apple.WebCore             	0x00000003bf971b28 WebCore::RenderLayer::updateLayerPositions(WebCore::RenderGeometryMap*, WTF::OptionSet<WebCore::RenderLayer::UpdateLayerPositionsFlag>) + 2248 (RenderLayer.cpp:1032)
9   com.apple.WebCore             	0x00000003bf971b28 WebCore::RenderLayer::updateLayerPositions(WebCore::RenderGeometryMap*, WTF::OptionSet<WebCore::RenderLayer::UpdateLayerPositionsFlag>) + 2248 (RenderLayer.cpp:1032)
10  com.apple.WebCore             	0x00000003bf971b28 WebCore::RenderLayer::updateLayerPositions(WebCore::RenderGeometryMap*, WTF::OptionSet<WebCore::RenderLayer::UpdateLayerPositionsFlag>) + 2248 (RenderLayer.cpp:1032)
11  com.apple.WebCore             	0x00000003bf971e0b WebCore::RenderLayer::updateLayerPositionsAfterLayout(bool, bool) + 219 (RenderLayer.cpp:931)
12  com.apple.WebCore             	0x00000003bef1179e WebCore::FrameView::didLayout(WTF::WeakPtr<WebCore::RenderElement, WTF::EmptyCounter>) + 142 (FrameView.cpp:1297)
13  com.apple.WebCore             	0x00000003bef06dac WebCore::FrameViewLayoutContext::layout() + 2828 (FrameViewLayoutContext.cpp:260)
14  com.apple.WebCore             	0x00000003bef1a8b0 WebCore::FrameView::updateContentsSize() + 112 (FrameView.cpp:2780)
15  com.apple.WebCore             	0x00000003bf15376c WebCore::ScrollView::updateScrollbars(WebCore::IntPoint const&) + 2668 (ScrollView.cpp:711)
16  com.apple.WebCore             	0x00000003bf155320 WebCore::ScrollView::setContentsSize(WebCore::IntSize const&) + 160 (ScrollView.cpp:403)
17  com.apple.WebCore             	0x00000003bef0c73f WebCore::FrameView::setContentsSize(WebCore::IntSize const&) + 111 (FrameView.cpp:590)
18  com.apple.WebCore             	0x00000003bef039bb WebCore::FrameView::adjustViewSize() + 747 (FrameView.cpp:622)
19  com.apple.WebCore             	0x00000003bef06c21 WebCore::FrameViewLayoutContext::layout() + 2433 (FrameViewLayoutContext.cpp:248)
...

[Radar WebKit Bug Importer]

<rdar://problem/83179970>

[Martin Robinson]

Created attachment 448730 [details]
Patch

[Nikolas Zimmermann]

Comment on attachment 448730 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=448730&action=review

Thanks Martin for the patch. Here's an unofficial review / some thoughts:

> Source/WebCore/rendering/RenderElement.h:88
> +    // Whether or not this RenderObject is a "transformable element" as defined by the css-transforms-1 specification.

Strictly speaking this is mixing terminology: The "transformable element" definition refers to DOM elements, whereas you attempt to define whether or not a RenderElement is transformable (whatever this means).
The name 'isTransformable' is quite broad, whereas you only check CSS transformations here -- there are RenderElements in the render tree that are transformed by other means, such as RenderSVG* (inheriting from RenderElement, via LegacyRenderSVGModelObject). However SVG transforms aren't considered in your isTransformable() definition... the name needs to be re-visited.

> Source/WebCore/rendering/RenderElement.h:484
> +    return isBoxModelObject() && !isRenderInline() && !isRenderTableCol();

This is an observable change: any RenderBoxModelObject derived renderer is now considered transformable (if it's not RenderInline-derived or RenderTableCol). Observable in the sense, that you're potentially influencing the RenderLayer hierarchy creation, where these canContainXXX methods are used. Therefore we need to be very careful with the conditions that are checked here.
You return true in isTransformable() for basically all RenderBox-derived classes, such as Render{Replaced|TableRow|TableSection|...} (therefore also LegacyRenderSVGRoot), except the two cases that you excluded: RenderTableCol/RenderInline. Are all the cases covered by WPT tests? (need to make a list of affected renderers...)

However, do we really need to define isTransformable here?

To answer if an element "can contain fixed position / absolutely positioned objects", one needs to find out if the given renderer can establish a containing block for its descendants, which happens to be the case for all "transformed elements", according to CSS Transforms Level 1: "For elements whose layout is governed by the CSS box model, any value other than none for the transform property also causes the element to establish a containing block for all descendants."

You could move both "isTransformable() && hasTransform()" into a establishesContainingBlockDueToTransformations() method, that returns false if no CSS transform property is set, or otherwise consult an "accept list", to see if the current renderer respects CSS transformations (there you can return false for all SVG enderers, except RenderSVGForeignObject). In this way you can also fold the "|| isSVGForeignObject()" check from canContainXY into the new establishesContainingBlockDueToTransformations() method.

Alternatively you could also obtain the containingBlock() for the renderer, and check it's == this, however this is way more expensive than some is<RenderXY>() checks + hasTransform(), and potentially dangerous during render tree building, so ... stay away from it :-)

(Side-Note: I naively expected both canContainXY methods to check the same: hasTransform not hasTransformRelatedProperty -- that needs a careful check first, why these checks are not identical)

[Martin Robinson]

(In reply to Nikolas Zimmermann from comment #3)
> Comment on attachment 448730 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=448730&action=review
> 
> Thanks Martin for the patch. Here's an unofficial review / some thoughts:

Thanks for the review! 

> > Source/WebCore/rendering/RenderElement.h:88
> > +    // Whether or not this RenderObject is a "transformable element" as defined by the css-transforms-1 specification.
> 
> Strictly speaking this is mixing terminology: The "transformable element"
> definition refers to DOM elements, whereas you attempt to define whether or
> not a RenderElement is transformable (whatever this means).
> The name 'isTransformable' is quite broad, whereas you only check CSS
> transformations here -- there are RenderElements in the render tree that are
> transformed by other means, such as RenderSVG* (inheriting from
> RenderElement, via LegacyRenderSVGModelObject). However SVG transforms
> aren't considered in your isTransformable() definition... the name needs to
> be re-visited.

This is a good point. 
 
> > Source/WebCore/rendering/RenderElement.h:484
> > +    return isBoxModelObject() && !isRenderInline() && !isRenderTableCol();
> 
> This is an observable change: any RenderBoxModelObject derived renderer is
> now considered transformable (if it's not RenderInline-derived or
> RenderTableCol). Observable in the sense, that you're potentially
> influencing the RenderLayer hierarchy creation, where these canContainXXX
> methods are used. Therefore we need to be very careful with the conditions
> that are checked here.
> You return true in isTransformable() for basically all RenderBox-derived
> classes, such as Render{Replaced|TableRow|TableSection|...} (therefore also
> LegacyRenderSVGRoot), except the two cases that you excluded:
> RenderTableCol/RenderInline. Are all the cases covered by WPT tests? (need
> to make a list of affected renderers...)

You're right, I think. It's best to target this fix as much as possible until there's test coverage for other cases.

> You could move both "isTransformable() && hasTransform()" into a
> establishesContainingBlockDueToTransformations() method, that returns false
> if no CSS transform property is set, or otherwise consult an "accept list",
> to see if the current renderer respects CSS transformations (there you can
> return false for all SVG enderers, except RenderSVGForeignObject). In this
> way you can also fold the "|| isSVGForeignObject()" check from canContainXY
> into the new establishesContainingBlockDueToTransformations() method.

I've taken this approach and just explicitly added table parts. There is testing for table parts and this change leads to a progression for these tests (though doesn't completely fix them).

> 
> Alternatively you could also obtain the containingBlock() for the renderer,
> and check it's == this, however this is way more expensive than some
> is<RenderXY>() checks + hasTransform(), and potentially dangerous during
> render tree building, so ... stay away from it :-)

I'm not sure this will work, because `containingBlock()` calls `canContainAbsolutelyPositionedObjects()` and `canContainFixedPositionObjects()`. :)

> 
> (Side-Note: I naively expected both canContainXY methods to check the same:
> hasTransform not hasTransformRelatedProperty -- that needs a careful check
> first, why these checks are not identical)

Totally agree here. This should be investigated and addressed if it's wrong (though in a different change).

[Martin Robinson]

Created attachment 448756 [details]
Patch

[Rob Buis]

Martin, you may want to unskip top-layer-ancestor-opacity-and-transform-crash.html in this patch when landing this patch (see https://bugs.webkit.org/show_bug.cgi?id=234315).

[Martin Robinson]

(In reply to Rob Buis from comment #6)
> Martin, you may want to unskip
> top-layer-ancestor-opacity-and-transform-crash.html in this patch when
> landing this patch (see https://bugs.webkit.org/show_bug.cgi?id=234315).

I took a look at this test and it seems like it's a slightly different issue. This one is about table elements which are transformed and the assertion failure you mention is about top layer elements with a transformed body. I suspect that the fix will be a bit different for that issue.

[EWS]

Tools/Scripts/svn-apply failed to apply attachment 448756 [details] to trunk.
Please resolve the conflicts and upload a new patch.

[Martin Robinson]

Created attachment 448838 [details]
Patch

[EWS]

Committed r287875 (245919@main): <https://commits.webkit.org/245919@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 448838 [details].