Bug 96919 - [Texmap] Potential crash in TextureMapperLayer because of referencing deleted mask/replica layer
Summary: [Texmap] Potential crash in TextureMapperLayer because of referencing deleted...
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: Balazs Kelemen
URL:
Keywords:
Depends on: 97561
Blocks:
  Show dependency treegraph
 
Reported: 2012-09-17 07:17 PDT by Balazs Kelemen
Modified: 2012-09-25 05:33 PDT (History)
5 users (show)

See Also:


Attachments
Patch (3.92 KB, patch)
2012-09-17 07:28 PDT, Balazs Kelemen
no flags Details | Formatted Diff | Diff
Patch (3.27 KB, patch)
2012-09-18 07:51 PDT, Balazs Kelemen
no flags Details | Formatted Diff | Diff
Patch (3.25 KB, patch)
2012-09-18 08:19 PDT, Balazs Kelemen
no flags Details | Formatted Diff | Diff
Patch (3.24 KB, patch)
2012-09-18 08:33 PDT, Balazs Kelemen
noam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Balazs Kelemen 2012-09-17 07:17:28 PDT
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.
Comment 1 Balazs Kelemen 2012-09-17 07:28:13 PDT
Created attachment 164392 [details]
Patch
Comment 2 Jocelyn Turcotte 2012-09-17 07:36:05 PDT
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?
Comment 3 Balazs Kelemen 2012-09-17 07:59:50 PDT
(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.
Comment 4 Noam Rosenthal 2012-09-17 08:11:45 PDT
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.
Comment 5 Balazs Kelemen 2012-09-17 08:26:49 PDT
(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.
Comment 6 Jocelyn Turcotte 2012-09-17 08:35:17 PDT
(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?
Comment 7 Jocelyn Turcotte 2012-09-17 08:37:57 PDT
(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.
Comment 8 Balazs Kelemen 2012-09-18 06:42:28 PDT
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.
Comment 9 Noam Rosenthal 2012-09-18 07:06:59 PDT
(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.
Comment 10 Balazs Kelemen 2012-09-18 07:51:54 PDT
Created attachment 164560 [details]
Patch
Comment 11 Noam Rosenthal 2012-09-18 07:54:54 PDT
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
Comment 12 Balazs Kelemen 2012-09-18 08:19:56 PDT
Created attachment 164563 [details]
Patch
Comment 13 Balazs Kelemen 2012-09-18 08:20:37 PDT
(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 :)
Comment 14 Early Warning System Bot 2012-09-18 08:30:59 PDT
Comment on attachment 164563 [details]
Patch

Attachment 164563 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13896225
Comment 15 Balazs Kelemen 2012-09-18 08:33:45 PDT
Created attachment 164566 [details]
Patch
Comment 16 Rafael Brandao 2012-09-18 11:56:03 PDT
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.
Comment 17 Rafael Brandao 2012-09-18 13:07:37 PDT
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.
Comment 18 Noam Rosenthal 2012-09-18 14:29:16 PDT
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.
Comment 19 Balazs Kelemen 2012-09-19 02:10:52 PDT
Comment on attachment 164566 [details]
Patch

Landed in http://trac.webkit.org/changeset/128980 with the HashSet->Vector change
Comment 20 Balazs Kelemen 2012-09-19 02:20:37 PDT
(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.
Comment 21 Csaba Osztrogonác 2012-09-25 05:33:19 PDT
(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