Bug 75883 - [Qt][WK2] WebProcess crashes when composited reflections/masks are present
Summary: [Qt][WK2] WebProcess crashes when composited reflections/masks are present
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: Qt
Depends on:
Blocks:
 
Reported: 2012-01-09 12:30 PST by Noam Rosenthal
Modified: 2012-02-09 06:27 PST (History)
4 users (show)

See Also:


Attachments
Patch (11.63 KB, patch)
2012-01-09 15:31 PST, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (11.63 KB, patch)
2012-01-10 07:21 PST, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch 1 (11.63 KB, patch)
2012-01-11 16:19 PST, Noam Rosenthal
no flags Details | Formatted Diff | Diff
atch 1: minor improvements to TextureMapperNode (11.58 KB, patch)
2012-01-11 16:27 PST, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch 2: Fix mask/reflection handling in Qt WebKit2 (8.45 KB, patch)
2012-01-11 17:14 PST, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch 1: minor improvements to TextureMapperNode (11.70 KB, patch)
2012-01-12 08:30 PST, Noam Rosenthal
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Noam Rosenthal 2012-01-09 12:30:24 PST
All files in LayoutTests/compositing/reflections cause the web-process to crash.Q
Comment 1 Noam Rosenthal 2012-01-09 15:31:50 PST
Created attachment 121733 [details]
Patch

Some of the layout tests still don't render correctly, but that can be fixed in a later patch.
Comment 2 Kenneth Rohde Christiansen 2012-01-10 05:11:17 PST
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?
Comment 3 Noam Rosenthal 2012-01-10 06:52:02 PST
(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).
Comment 4 Noam Rosenthal 2012-01-10 07:21:58 PST
Created attachment 121843 [details]
Patch
Comment 5 Simon Hausmann 2012-01-11 06:21:46 PST
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?
Comment 6 Noam Rosenthal 2012-01-11 06:35:44 PST
> > 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.
Comment 7 Noam Rosenthal 2012-01-11 16:19:25 PST
Created attachment 122118 [details]
Patch 1
Comment 8 Noam Rosenthal 2012-01-11 16:27:07 PST
Created attachment 122123 [details]
atch 1: minor improvements to TextureMapperNode
Comment 9 Noam Rosenthal 2012-01-11 17:14:28 PST
Created attachment 122137 [details]
Patch 2: Fix mask/reflection handling in Qt WebKit2
Comment 10 Kenneth Rohde Christiansen 2012-01-12 01:50:22 PST
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.
Comment 11 Noam Rosenthal 2012-01-12 08:30:16 PST
Created attachment 122250 [details]
Patch 1: minor improvements to TextureMapperNode

Addressed Kenneth's comments.
Comment 12 WebKit Review Bot 2012-01-12 09:27:58 PST
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>
Comment 13 Noam Rosenthal 2012-01-14 10:25:50 PST
Comment on attachment 122250 [details]
Patch 1: minor improvements to TextureMapperNode

Filed another bug, 76336