Prepare CompositorWorker for per thread heap

third_party/WebKit/Source/web/CompositorMutatorImpl.cpp

Index: third_party/WebKit/Source/web/CompositorMutatorImpl.cpp
diff --git a/third_party/WebKit/Source/web/CompositorMutatorImpl.cpp b/third_party/WebKit/Source/web/CompositorMutatorImpl.cpp
index eddb7cb5972855c792cb2a2e48e854c5f32239ae..a3b057e7ab37c7ce2546faa79b48a697773ce00d 100644
--- a/third_party/WebKit/Source/web/CompositorMutatorImpl.cpp
+++ b/third_party/WebKit/Source/web/CompositorMutatorImpl.cpp
@@ -64,8 +64,9 @@ bool CompositorMutatorImpl::mutate(double monotonicTimeNow, CompositorMutableSta
     // TODO(vollick): we should avoid executing the animation frame
     // callbacks if none of the proxies in the global scope are affected by
     // m_mutations.
-    for (CompositorProxyClientImpl* client : m_proxyClients) {
-        if (client->mutate(monotonicTimeNow, stateProvider))
+    for (auto& it : m_proxyClients) {
+        CrossThreadPersistent<CompositorProxyClientImpl> client(it);
+        if (client && client->mutate(monotonicTimeNow, stateProvider))
             needToReinvoke = true;
     }
 
@@ -75,7 +76,21 @@ bool CompositorMutatorImpl::mutate(double monotonicTimeNow, CompositorMutableSta
 void CompositorMutatorImpl::registerProxyClient(CompositorProxyClientImpl* client)
 {
     TRACE_EVENT0("compositor-worker", "CompositorMutatorImpl::registerClient");
-    m_proxyClients.add(client);
+    CrossThreadWeakPersistent<CompositorProxyClientImpl>* insertPosition = nullptr;
+    for (auto& it : m_proxyClients) {

[haraken, 2016/08/22 14:24:44]

Would it make more sense to provide a helper class like this?

  CrossThreadPersistentScope scope(m_proxyClients);  // This strongifies handles
in the map.
  ...;

[keishi, 2016/09/02 06:41:55]

Strongifying all items at once is hard, and this pattern is only used here for
now so I would like to leave this as is.

+        CrossThreadPersistent<CompositorProxyClientImpl> protect(it);

[haraken, 2016/08/22 14:24:44]

I'm just curious but how did you find that you need a protecting pointer here?
If you miss the protecting pointer, will you hit the DCHECK in the runtime
verification?

[keishi, 2016/09/02 06:41:55]

I think the only safe way to use CrossThreadWeakPersistent (in a non-owner
thread), is by protecting it with a CrossThreadPersistent first. If you don't
protect it the object might disappear while using it. Also I think we need to
use the special CrossThreadPersistent::protectWeak() which locks the
crossThreadPersistentRegion before assigning, so the object isn't destroyed
while assigning. (I was trying to use CrossThreadPersistent constructor to
trigger the locking but I couldn't trigger the right constructor because
CrossThreadWeakPersistent could be implicitly cast to a raw pointer)

I added DCHECK to CrossThreadWeakPersistent accessor methods to avoid this
mistake.

[haraken, 2016/09/02 09:39:17]

Ah, the DCHECK looks clever!

However, maybe would it be better to make CrossThreadWeakPersistent::operator*
unconditionally return CrossThreadPersistent? I think it's *always* unsafe to
return a raw pointer from CrossThreadWeakPersistent::operator* because nothing
protects the raw pointer on the stack.

[keishi, 2016/09/07 10:14:42]

I was worried about this so I would love to do this, 
but we can't change the return type for CrossThreadWeakPersistent::operator* and
base::Bind uses that method so I can't remove it.

+        if (!protect && !insertPosition) {
+            insertPosition = ⁢
+#if !DCHECK_IS_ON()
+            break;
+#endif
+        }
+        DCHECK_NE(protect, client);
+    }
+    if (insertPosition)
+        *insertPosition = client;
+    else
+        m_proxyClients.append(client);
     setNeedsMutate();
 }