Bug 101341 – [CoordinatedGraphics] Access to LayerTreeRenderer::m_renderQueue should be thread safe

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

[Balazs Kelemen]

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.

[Balazs Kelemen]

Created attachment 172562 [details]
Patch

[Balazs Kelemen]

Comment on attachment 172562 [details]
Patch

Clearing flags on attachment: 172562

Committed r133599: <http://trac.webkit.org/changeset/133599>

[Balazs Kelemen]

All reviewed patches have been landed.  Closing bug.

[Rafael Brandao]

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?

[Balazs Kelemen]

(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.

[Rafael Brandao]

Reopening to attach new patch.

[Rafael Brandao]

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.

[WebKit Review Bot]

Comment on attachment 176037 [details]
Patch

Clearing flags on attachment: 176037

Committed r135748: <http://trac.webkit.org/changeset/135748>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.