Bug 101341 - [CoordinatedGraphics] Access to LayerTreeRenderer::m_renderQueue should be thread safe
Summary: [CoordinatedGraphics] Access to LayerTreeRenderer::m_renderQueue should be th...
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:
Blocks:
 
Reported: 2012-11-06 05:52 PST by Balazs Kelemen
Modified: 2012-11-26 11:46 PST (History)
6 users (show)

See Also:

Attachments
Patch (3.01 KB, patch)
2012-11-06 05:56 PST, Balazs Kelemen 		no flags Details | Formatted Diff | Diff
Patch (1.53 KB, patch)
2012-11-26 11:07 PST, Rafael Brandao 		no flags Details | Formatted Diff | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Balazs Kelemen 2012-11-06 05:52:24 PST 
If I'm not wrong, access to the queue can be a race condition across syncRemoteContent and some methods of LayerTreeCoordinatorProxy that update it from the main thread, like setContentsSize or setVisibleContentsRect. Those are used by public API so we should not take any preassumption about when will they be called.
Comment 1 Balazs Kelemen 2012-11-06 05:56:37 PST 
Created attachment 172562 [details]
Patch
Comment 2 Balazs Kelemen 2012-11-06 06:40:35 PST 
Comment on attachment 172562 [details]
Patch

Clearing flags on attachment: 172562

Committed r133599: <http://trac.webkit.org/changeset/133599>
Comment 3 Balazs Kelemen 2012-11-06 06:40:39 PST 
All reviewed patches have been landed.  Closing bug.
Comment 4 Rafael Brandao 2012-11-26 06:03:02 PST 
Comment on attachment 172562 [details]
Patch

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

> Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:476
>      m_renderQueue.clear();

This call here to clear the m_renderQueue should be removed. When we do the swap between a new vector and the m_renderQueue, the resulting queue on m_renderQueue is an empty one. And still you will do that when you have the lock, rather than releasing the lock and doing it afterwards. Should I upload a patch to fix this?
Comment 5 Balazs Kelemen 2012-11-26 10:39:34 PST 
(In reply to comment #4)
> (From update of attachment 172562 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=172562&action=review
> 
> > Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:476
> >      m_renderQueue.clear();
> 
> This call here to clear the m_renderQueue should be removed. When we do the swap between a new vector and the m_renderQueue, the resulting queue on m_renderQueue is an empty one. And still you will do that when you have the lock, rather than releasing the lock and doing it afterwards. Should I upload a patch to fix this?

Just removing the clear is enough. There is no need to hold the lock after filling the local vector. It is not a problem if we add new tasks in the main thread, those will be flushed later. Fell free to update the patch.
Comment 6 Rafael Brandao 2012-11-26 11:07:16 PST 
Reopening to attach new patch.
Comment 7 Rafael Brandao 2012-11-26 11:07:20 PST 
Created attachment 176037 [details]
Patch

I've decided to keep this follow up patch here in the same bug to give a better context. I'm just removing the unnecessary clear of m_renderQueue with the same reasoning of this bug title.
Comment 8 WebKit Review Bot 2012-11-26 11:46:18 PST 
Comment on attachment 176037 [details]
Patch

Clearing flags on attachment: 176037

Committed r135748: <http://trac.webkit.org/changeset/135748>
Comment 9 WebKit Review Bot 2012-11-26 11:46:22 PST 
All reviewed patches have been landed.  Closing bug.