Bug 197695

Summary: fast/hidpi/video-controls-in-hidpi.html sometimes asserts in WK1
Product: WebKit Reporter: Simon Fraser (smfr) <simon.fraser>
Component: CompositingAssignee: Simon Fraser (smfr) <simon.fraser>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, simon.fraser, webkit-bug-importer, zalan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=126160
Attachments:
Description Flags
Patch none

Description Simon Fraser (smfr) 2019-05-08 10:17:52 PDT
With the patch from bug 197561, fast/hidpi/video-controls-in-hidpi.html sometimes asserts on EWS (I have not been able to reproduce).
Comment 1 Simon Fraser (smfr) 2019-05-08 10:18:25 PDT
The assertion is:
        ASSERT(!parentClipRect.isInfinite());
Comment 2 Simon Fraser (smfr) 2019-05-08 21:24:07 PDT
This code runs when we have a m_ancestorClippingLayer, and we make one based on the result of RenderLayerCompositor::clippedByAncestor(), which uses:

layer.backgroundClipRect(RenderLayer::ClipRectsContext(computeClipRoot, TemporaryClipRects)).isInfinite()

However, RenderLayerBacking::computeParentGraphicsLayerRect() is doing:

        ShouldRespectOverflowClip shouldRespectOverflowClip = compositedAncestor->isolatesCompositedBlending() ? RespectOverflowClip : IgnoreOverflowClip;
        RenderLayer::ClipRectsContext clipRectsContext(compositedAncestor, TemporaryClipRects, IgnoreOverlayScrollbarSize, shouldRespectOverflowClip);
        LayoutRect parentClipRect = m_owningLayer.backgroundClipRect(clipRectsContext).rect(); // FIXME: Incorrect for CSS regions.

so 'shouldRespectOverflowClip' differs.
Comment 3 Simon Fraser (smfr) 2019-05-08 21:24:14 PDT
*** Bug 197731 has been marked as a duplicate of this bug. ***
Comment 4 Simon Fraser (smfr) 2019-05-08 21:24:37 PDT
That code came from https://trac.webkit.org/changeset/168314/webkit
Comment 5 Simon Fraser (smfr) 2019-05-08 21:49:53 PDT
accessibility/media-element.html also hits this
Comment 6 Simon Fraser (smfr) 2019-05-08 22:37:34 PDT
One thing to note here is that normally we don't have an m_ancestorLayer if our clipping ancestor is a stacking context, but with a composited mix-blend-mode child, RenderLayerCompositor::clippedByAncestor() follows a different code path, and can give us an m_ancestorClippingLayer. I don't know why.
Comment 7 Simon Fraser (smfr) 2019-05-08 22:48:00 PDT
This bug is about maintenance of the isolatesCompositedBlending() bit on RenderLayer vs. the maintenance of m_ancestorClippingLayer.

If compositedAncestor->isolatesCompositedBlending() was true at the last compositing update, we made a m_ancestorClippingLayer. At some point another composited sibling with blending became non-composited, so compositedAncestor->isolatesCompositedBlending() is no longer true but we didn't remove the m_ancestorClippingLayer.
Comment 8 Simon Fraser (smfr) 2019-05-08 22:50:10 PDT
isolatesCompositedBlending depends on both m_hasNotIsolatedCompositedBlendingDescendants and isCSSStackingContext
Comment 9 Simon Fraser (smfr) 2019-05-08 23:07:29 PDT
The fix is something like:

index 15479365f2edf9776a01e4f4e70c792921824975..8532ac6c3acb2df5eb75a5d6b43981d2cc5c4eaa 100644
--- a/Source/WebCore/rendering/RenderLayerCompositor.cpp
+++ b/Source/WebCore/rendering/RenderLayerCompositor.cpp
@@ -1022,8 +1022,13 @@ void RenderLayerCompositor::computeCompositingRequirements(RenderLayer* ancestor
         addToOverlapMap(overlapMap, layer, layerExtent);
 
 #if ENABLE(CSS_COMPOSITING)
+    bool isolatedCompositedBlending = layer.isolatesCompositedBlending();
     layer.setHasNotIsolatedCompositedBlendingDescendants(childState.hasNotIsolatedCompositedBlendingDescendants);
     ASSERT(!layer.hasNotIsolatedCompositedBlendingDescendants() || layer.hasNotIsolatedBlendingDescendants());
+    if (layer.isolatesCompositedBlending() != isolatedCompositedBlending) {
+        // isolatedCompositedBlending affects the answer to clippedByAncestor().
+        layer.setChildrenNeedCompositingGeometryUpdate();
+    }
 #endif
     // Now check for reasons to become composited that depend on the state of descendant layers.
     RenderLayer::IndirectCompositingReason indirectCompositingReason;
Comment 10 Simon Fraser (smfr) 2019-05-09 07:14:29 PDT
Created attachment 369491 [details]
Patch
Comment 11 Radar WebKit Bug Importer 2019-05-09 07:38:10 PDT
<rdar://problem/50621407>
Comment 12 WebKit Commit Bot 2019-05-09 11:26:30 PDT
Comment on attachment 369491 [details]
Patch

Clearing flags on attachment: 369491

Committed r245147: <https://trac.webkit.org/changeset/245147>
Comment 13 WebKit Commit Bot 2019-05-09 11:26:32 PDT
All reviewed patches have been landed.  Closing bug.