Aura: Resize locks with --ui-enable-threaded-compositing

cc/thread_proxy.cc

Index: cc/thread_proxy.cc
diff --git a/cc/thread_proxy.cc b/cc/thread_proxy.cc
index ee08f77cb3be14c3c6a6506e27944f14a1ccc0c1..725c461e7180e55687b6654f01c4436b2bd9eb63 100644
--- a/cc/thread_proxy.cc
+++ b/cc/thread_proxy.cc
@@ -53,6 +53,7 @@ ThreadProxy::ThreadProxy(LayerTreeHost* layerTreeHost)
     , m_nextFrameIsNewlyCommittedFrameOnImplThread(false)
     , m_renderVSyncEnabled(layerTreeHost->settings().renderVSyncEnabled)
     , m_totalCommitCount(0)
+    , m_deferCommits(false)
 {
     TRACE_EVENT0("cc", "ThreadProxy::ThreadProxy");
     DCHECK(isMainThread());
@@ -70,6 +71,7 @@ bool ThreadProxy::compositeAndReadback(void *pixels, const IntRect& rect)
     TRACE_EVENT0("cc", "ThreadPRoxy::compositeAndReadback");
     DCHECK(isMainThread());
     DCHECK(m_layerTreeHost);
+    DCHECK(!m_deferCommits);
 
     if (!m_layerTreeHost->initializeRendererIfNeeded()) {
         TRACE_EVENT0("cc", "compositeAndReadback_EarlyOut_LR_Uninitialized");
@@ -131,6 +133,7 @@ void ThreadProxy::requestStartPageScaleAnimationOnImplThread(IntSize targetPosit
 void ThreadProxy::finishAllRendering()
 {
     DCHECK(Proxy::isMainThread());
+    DCHECK(!m_deferCommits);
 
     // Make sure all GL drawing is finished on the impl thread.
     DebugScopedSetMainThreadBlocked mainThreadBlocked;
@@ -373,6 +376,21 @@ void ThreadProxy::setNeedsRedraw()
     Proxy::implThread()->postTask(createThreadTask(this, &ThreadProxy::setNeedsRedrawOnImplThread));
 }
 
+void ThreadProxy::setDeferCommits(bool deferCommits)
+{
+    DCHECK(isMainThread());
+    DCHECK_NE(m_deferCommits, deferCommits);
+    m_deferCommits = deferCommits;
+
+    if (m_deferCommits)
+        TRACE_EVENT_ASYNC_BEGIN0("cc", "ThreadProxy::setDeferCommits", this);
+    else
+        TRACE_EVENT_ASYNC_END0("cc", "ThreadProxy::setDeferCommits", this);
+
+    if (!m_deferCommits && m_pendingBeginFrameRequest)

[piman, 2012/10/25 00:14:08]

There is a race condition here.
m_pendingBeginFrameRequest is set by scheduledActionBeginFrame on the impl
thread and then the struct is filled up - without a lock. However it's not meant
to be seen by the main thread only after the beginFrame task is posted - adding
an explicit coherency-ensuring ordering relationship
scheduledActionBeginFrame->postTask->runTask->beginFrame.

You can construct a case where the impl thread has set
m_pendingBeginFrameRequest, then the main thread calls setDeferCommits(false)
before the task is posted, and then executes beginFrame before the struct is
filled up (and visible by the main thread) causing inconsistencies.

You can probably solve it by adding a bool, that you would set in beginFrame
when commits are deferred, and here post a task (and reset it) when it is set.
That would guarantee that there's only a single beginFrame "in flight", and so
it would never see inconsistent state.

[jonathan.backer, 2012/10/25 15:19:04]

Nice catch! That would have been impossible to debug.

+        m_mainThreadProxy->postTask(createThreadTask(this, &ThreadProxy::beginFrame));
+}
+
 bool ThreadProxy::commitRequested() const
 {
     DCHECK(isMainThread());
@@ -487,6 +505,12 @@ void ThreadProxy::beginFrame()
     if (!m_layerTreeHost)
         return;
 
+    if (m_deferCommits) {
+        TRACE_EVENT0("cc", "EarlyOut_DeferCommits");
+        m_layerTreeHost->didDeferCommit();
+        return;
+    }
+
     if (!m_pendingBeginFrameRequest) {
         TRACE_EVENT0("cc", "EarlyOut_StaleBeginFrameMessage");
         return;