Bug 96919 – [Texmap] Potential crash in TextureMapperLayer because of referencing deleted mask/replica layer

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Balazs Kelemen]

When deleting a GraphicsLayerTextureMapper we remove the associated TextureMapperLayer from the layer tree but other layers can still refer to it with m_effectTarget, m_state.maskLayer or m_state.replicaLayer. I don't know how to reproduce it on trunk, I found it when working on pixel tests (bug 90394 and bug 95992). With those patches compositing/masks tests are crashing in TextureMapperLayer::paintRecursive because we refer to an already deleted object with m_state.maskLayer. I believe the solution here is to traverse the whole tree when deleting a layer and null out all pointers to it.

[Balazs Kelemen]

Created attachment 164392 [details]
Patch

[Jocelyn Turcotte]

Comment on attachment 164392 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        m_state.maskLayer or m_state.replicaLayer. For this reason we should traverse the whole tree

Would there be any way we could find a more efficient solution?

[Balazs Kelemen]

(In reply to comment #2)
> (From update of attachment 164392 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=164392&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        m_state.maskLayer or m_state.replicaLayer. For this reason we should traverse the whole tree
> 
> Would there be any way we could find a more efficient solution?

I guess we could add members like maskedLayer and replicatedLayer but I'm not sure it is something that need to be optimized since a typical layer tree is just a few nodes.

[Noam Rosenthal]

Comment on attachment 164392 [details]
Patch

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

>>> Source/WebCore/ChangeLog:9
>>> +        m_state.maskLayer or m_state.replicaLayer. For this reason we should traverse the whole tree
>> 
>> Would there be any way we could find a more efficient solution?
> 
> I guess we could add members like maskedLayer and replicatedLayer but I'm not sure it is something that need to be optimized since a typical layer tree is just a few nodes.

We can also use an observer pattern, where the mask/replica layer observes the effectTarget and get's a virtual callback when it's aboutToBeDeleted.

[Balazs Kelemen]

(In reply to comment #4)
> (From update of attachment 164392 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=164392&action=review
> 
> >>> Source/WebCore/ChangeLog:9
> >>> +        m_state.maskLayer or m_state.replicaLayer. For this reason we should traverse the whole tree
> >> 
> >> Would there be any way we could find a more efficient solution?
> > 
> > I guess we could add members like maskedLayer and replicatedLayer but I'm not sure it is something that need to be optimized since a typical layer tree is just a few nodes.
> 
> We can also use an observer pattern, where the mask/replica layer observes the effectTarget and get's a virtual callback when it's aboutToBeDeleted.

I did not know that this is the role of the effectTarget. But if we already have a pointer to it, I think we could simply use it directly without an observer pattern.

[Jocelyn Turcotte]

(In reply to comment #3)
> I guess we could add members like maskedLayer and replicatedLayer but I'm not sure it is something that need to be optimized since a typical layer tree is just a few nodes.

Isn't this->m_state.maskLayer->m_effectTarget == this?

[Jocelyn Turcotte]

(In reply to comment #5)
> I did not know that this is the role of the effectTarget. But if we already have a pointer to it, I think we could simply use it directly without an observer pattern.

Ah yep, that's what I was trying to say, ignore my comment.

[Balazs Kelemen]

The basic issue here is that if we have a deletelayer message, and we try to paint before didRenderFrame when all layers are synced (for example the layer that has the deleted one as a mask layer) than our state is not synced correctly. In my use case I paint on every setNeedsDisplay which is called after every message from the LayerTreeCoordinator. However I can imagine that a broken state could occur if the painting is controlled by the scene graph as well. One solution could be to delay every message to be sent until flushPendingLayerChanges, or delay the setNeedsDisplay until didRenderFrame in the UI proc, or maybe we could fix all these cases on the UI side like I did in this patch for the mask layer. I wonder what is the best solution in your opinion.

[Noam Rosenthal]

(In reply to comment #8)
> The basic issue here is that if we have a deletelayer message, and we try to paint before didRenderFrame when all layers are synced (for example the layer that has the deleted one as a mask layer) than our state is not synced correctly. In my use case I paint on every setNeedsDisplay which is called after every message from the LayerTreeCoordinator. However I can imagine that a broken state could occur if the painting is controlled by the scene graph as well. One solution could be to delay every message to be sent until flushPendingLayerChanges, or delay the setNeedsDisplay until didRenderFrame in the UI proc, or maybe we could fix all these cases on the UI side like I did in this patch for the mask layer. I wonder what is the best solution in your opinion.

We should delay the actual deleting of the mask layer till the beginning of LayerTreeRenderer::flushLayerChanges(). 

In the web process this happens anyway, because layer deletion and syncing is all done synchronously, but in the ui process, as you say, the user may scroll in the middle of those messages.

[Balazs Kelemen]

Created attachment 164560 [details]
Patch

[Noam Rosenthal]

Comment on attachment 164560 [details]
Patch

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

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/LayerTreeCoordinator.h:130
> +    HashSet<WebLayerID> m_deletedLayersNeedSync;

How about m_layersToDelete

[Balazs Kelemen]

Created attachment 164563 [details]
Patch

[Balazs Kelemen]

(In reply to comment #11)
> (From update of attachment 164560 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=164560&action=review
> 
> > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/LayerTreeCoordinator.h:130
> > +    HashSet<WebLayerID> m_deletedLayersNeedSync;
> 
> How about m_layersToDelete

I used m_detachedLayers as we agreed on that on IRC :)

[Early Warning System Bot]

Comment on attachment 164563 [details]
Patch

Attachment 164563 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13896225

[Balazs Kelemen]

Created attachment 164566 [details]
Patch

[Rafael Brandao]

Comment on attachment 164566 [details]
Patch

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

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/LayerTreeCoordinator.h:130
> +    HashSet<WebLayerID> m_detachedLayers;

Is there any need to make this a HashSet? Wouldn't a queue be enough? The reasoning behind it is not clear just by looking into this patch. If this is correct, then you should leave a note on your ChangeLog explaining it.

[Rafael Brandao]

By the way is there's anyway to actually reproduce this crash (perhaps slowing down some calls?). I don't remember reaching this crash experimenting on bug #93536, but it has been a while ago.

[Noam Rosenthal]

Comment on attachment 164566 [details]
Patch

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

>> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/LayerTreeCoordinator.h:130
>> +    HashSet<WebLayerID> m_detachedLayers;
> 
> Is there any need to make this a HashSet? Wouldn't a queue be enough? The reasoning behind it is not clear just by looking into this patch. If this is correct, then you should leave a note on your ChangeLog explaining it.

You're right, it should be a Vector.

[Balazs Kelemen]

Comment on attachment 164566 [details]
Patch

Landed in http://trac.webkit.org/changeset/128980 with the HashSet->Vector change

[Balazs Kelemen]

(In reply to comment #17)
> By the way is there's anyway to actually reproduce this crash (perhaps slowing down some calls?). I don't remember reaching this crash experimenting on bug #93536, but it has been a while ago.

Apply the patch from bug 90394 :) The way how WTR works now does not reproduce this. Maybe running something with mask layers in MiniBrowser, I'm not sure.

[Csaba Osztrogonác]

(In reply to comment #19)
> (From update of attachment 164566 [details])
> Landed in http://trac.webkit.org/changeset/128980 with the HashSet->Vector change

It broke all QRawWebView API test. Could you check it, please?
Here is the new bug report: https://bugs.webkit.org/show_bug.cgi?id=97561