Prepare CompositorWorker for per thread heap

third_party/WebKit/Source/web/CompositorMutatorImpl.cpp

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "web/CompositorMutatorImpl.h"
 
 #include "core/animation/CustomCompositorAnimationManager.h"
 #include "core/dom/CompositorProxy.h"
 #include "platform/CrossThreadFunctional.h"
 #include "platform/TraceEvent.h"
@@ skipping 46 matching lines @@
     return new CompositorMutatorImpl();
 }
 
 bool CompositorMutatorImpl::mutate(double monotonicTimeNow, CompositorMutableStateProvider* stateProvider)
 {
     TRACE_EVENT0("compositor-worker", "CompositorMutatorImpl::mutate");
     bool needToReinvoke = false;
     // 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;
     }
 
     return needToReinvoke;
 }
 
 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();
 }
 
 void CompositorMutatorImpl::setNeedsMutate()
 {
     TRACE_EVENT0("compositor-worker", "CompositorMutatorImpl::setNeedsMutate");
     m_client->setNeedsMutate();
 }
 
 } // namespace blink