Bug 75883 – [Qt][WK2] WebProcess crashes when composited reflections/masks are present

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Noam Rosenthal]

All files in LayoutTests/compositing/reflections cause the web-process to crash.Q

[Noam Rosenthal]

Created attachment 121733 [details]
Patch

Some of the layout tests still don't render correctly, but that can be fixed in a later patch.

[Kenneth Rohde Christiansen]

Comment on attachment 121733 [details]
Patch

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

> Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:254
> +        HashMap<int, ExternallyManagedTile>::iterator mapEnd = m_externallyManagedTiles.end();
> +        for (HashMap<int, ExternallyManagedTile>::iterator mapIterator = m_externallyManagedTiles.begin(); mapIterator != mapEnd; ++mapIterator) {

I think using 'end' and 'it' would be sufficient. Very common pattern in webkit

> Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:382
> +
> +    if (layer)
> +        toWebGraphicsLayer(layer)->setLayerTreeTileClient(layerTreeTileClient());

you dont need to setSize here? like in setMaskLayer?

[Noam Rosenthal]

(In reply to comment #2)
> (From update of attachment 121733 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=121733&action=review
> 
> > Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:254
> > +        HashMap<int, ExternallyManagedTile>::iterator mapEnd = m_externallyManagedTiles.end();
> > +        for (HashMap<int, ExternallyManagedTile>::iterator mapIterator = m_externallyManagedTiles.begin(); mapIterator != mapEnd; ++mapIterator) {
> 
> I think using 'end' and 'it' would be sufficient. Very common pattern in webkit
> 
> > Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:382
> > +
> > +    if (layer)
> > +        toWebGraphicsLayer(layer)->setLayerTreeTileClient(layerTreeTileClient());
> 
> you dont need to setSize here? like in setMaskLayer?

No, since reflection layers don't have content (they always draw their parent's content).

[Noam Rosenthal]

Created attachment 121843 [details]
Patch

[Simon Hausmann]

Comment on attachment 121843 [details]
Patch

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

Most of it makes sense to me, but I have a couple of questions where answers would help me to understand your patch better :)

> Source/WebCore/ChangeLog:13
> +        (WebCore::TextureMapperNode::collectVisibleContentsRects):

Can you elaborate about this part of your change?

> Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:257
> +    if (m_state.tileOwnership == ExternallyManagedTiles) {
> +        HashMap<int, ExternallyManagedTile>::iterator end = m_externallyManagedTiles.end();
> +        for (HashMap<int, ExternallyManagedTile>::iterator mapIterator = m_externallyManagedTiles.begin(); mapIterator != end; ++mapIterator) {
> +            if (mapIterator->second.frontBuffer.texture)
> +                return mapIterator->second.frontBuffer.texture.get();
> +        }

So this simply returns the texture of the first tile that has one, right?

> Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:382
> +    if (layer)
> +        toWebGraphicsLayer(layer)->setLayerTreeTileClient(layerTreeTileClient());

I notice that setReplicatedByLayer sets the layer tree client before notifyChange() and setMaskLayer() sets it afterwards. Is there any chance that could make a difference? (seems unlikely to me, but you know the code better, so I thought I'd mention it :)

> Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:414
> -    if (replicaLayer())
> -        replicaLayer()->syncCompositingState(rect);
> -    if (maskLayer())
> -        maskLayer()->syncCompositingState(rect);
> +
> +    if (WebGraphicsLayer* mask = toWebGraphicsLayer(maskLayer()))
> +        mask->syncCompositingStateForThisLayerOnly();
> +
> +    if (WebGraphicsLayer* replica = toWebGraphicsLayer(replicaLayer()))
> +        replica->syncCompositingStateForThisLayerOnly();

Aha, is this an optimization based on the fact that mask or replica layers cannot have children?

> Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:536
> -    if (m_image)
> +    if (!drawsContent())

This as well as the removal of the comment are subtle ;). Can you explain these two changes?

[Noam Rosenthal]

> > Source/WebCore/ChangeLog:13
> > +        (WebCore::TextureMapperNode::collectVisibleContentsRects):
> 
> Can you elaborate about this part of your change?
Replicas don't have their own content, so we don't need to check for their visible rect.
Masks always have the same visible rect as the masked item, they don't have their own visible rect, since they don't have their own geometry.

> 
> > Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:257
> > +        }
> 
> So this simply returns the texture of the first tile that has one, right?
Yes.

> I notice that setReplicatedByLayer sets the layer tree client before notifyChange() and setMaskLayer() sets it afterwards. Is there any chance that could make a difference? (seems unlikely to me, but you know the code better, so I thought I'd mention it :)
I don't think so. But I'll make it more consistent.

> 
> > Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:414
> > -    if (replicaLayer())
> > -        replicaLayer()->syncCompositingState(rect);
> > -    if (maskLayer())
> > -        maskLayer()->syncCompositingState(rect);
> > +
> > +    if (WebGraphicsLayer* mask = toWebGraphicsLayer(maskLayer()))
> > +        mask->syncCompositingStateForThisLayerOnly();
> > +
> > +    if (WebGraphicsLayer* replica = toWebGraphicsLayer(replicaLayer()))
> > +        replica->syncCompositingStateForThisLayerOnly();
> 
> Aha, is this an optimization based on the fact that mask or replica layers cannot have children?
Yes.

> 
> > Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:536
> > -    if (m_image)
> > +    if (!drawsContent())
> 
> This as well as the removal of the comment are subtle ;). Can you explain these two changes?
I think 75882 explains it better. I'll make sure these m_image changes and related comments are all in 75882.

[Noam Rosenthal]

Created attachment 122118 [details]
Patch 1

[Noam Rosenthal]

Created attachment 122123 [details]
atch 1: minor improvements to TextureMapperNode

[Noam Rosenthal]

Created attachment 122137 [details]
Patch 2: Fix mask/reflection handling in Qt WebKit2

[Kenneth Rohde Christiansen]

Comment on attachment 122123 [details]
atch 1: minor improvements to TextureMapperNode

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

> Source/WebCore/ChangeLog:14
> +        2. Rename TextureMapperNode::texture() to getFirstContentTextureForMask(), and allow the
> +           texture to come from the externally-managed tiles.

isn't the word find* more used for this? like findFirstContentTextureForMask?

> Source/WebCore/platform/graphics/texmap/TextureMapper.cpp:34
> +        // A texture that's not used would have two references - the reference inside the vector, and the "texture" local variable.

unused? but the comment is confusing as you check for > 2

> Source/WebCore/platform/graphics/texmap/TextureMapper.h:129
> -    virtual void releaseTextureToPool(BitmapTexture* surface);
>      virtual PassRefPtr<BitmapTexture> acquireTextureFromPool(const IntSize&);

Maybe add a comment here about how they are released.

[Noam Rosenthal]

Created attachment 122250 [details]
Patch 1: minor improvements to TextureMapperNode

Addressed Kenneth's comments.

[WebKit Review Bot]

Comment on attachment 122137 [details]
Patch 2: Fix mask/reflection handling in Qt WebKit2

Clearing flags on attachment: 122137

Committed r104832: <http://trac.webkit.org/changeset/104832>

[Noam Rosenthal]

Comment on attachment 122250 [details]
Patch 1: minor improvements to TextureMapperNode

Filed another bug, 76336