Fix DelegatedFrameResourceCollection thread safety

Don't keep a ref to DelegatedFrameResourceCollection inside the return resources
callback, because it is called from a thread.

BUG=None
R=danakj@chromium.org

Review URL: https://codereview.chromium.org/47703005

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@231915 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 172 insertions, 15 deletions

diff --git a/cc/layers/delegated_frame_resource_collection.cc b/cc/layers/delegated_frame_resource_collection.cc
index cd2633d..5ca4eda 100644
--- a/cc/layers/delegated_frame_resource_collection.cc
+++ b/cc/layers/delegated_frame_resource_collection.cc
@@ -12,7 +12,8 @@ namespace cc {
 DelegatedFrameResourceCollection::DelegatedFrameResourceCollection()
     : client_(NULL),
       main_thread_runner_(BlockingTaskRunner::current()),
-      lost_all_resources_(false) {
+      lost_all_resources_(false),
+      weak_ptr_factory_(this) {
   DCHECK(main_thread_checker_.CalledOnValidThread());
 }
 
@@ -112,21 +113,21 @@ void DelegatedFrameResourceCollection::RefResources(
     resource_id_ref_count_map_[resources[i].id].refs_to_wait_for++;
 }
 
-ReturnCallback
-DelegatedFrameResourceCollection::GetReturnResourcesCallbackForImplThread() {
-  return base::Bind(
-      &DelegatedFrameResourceCollection::UnrefResourcesOnImplThread,
-      this,
-      main_thread_runner_);
-}
-
-void DelegatedFrameResourceCollection::UnrefResourcesOnImplThread(
+static void UnrefResourcesOnImplThread(
+    base::WeakPtr<DelegatedFrameResourceCollection> self,
     scoped_refptr<BlockingTaskRunner> main_thread_runner,
     const ReturnedResourceArray& returned) {
   main_thread_runner->PostTask(
       FROM_HERE,
       base::Bind(
-          &DelegatedFrameResourceCollection::UnrefResources, this, returned));
+          &DelegatedFrameResourceCollection::UnrefResources, self, returned));
+}
+
+ReturnCallback
+DelegatedFrameResourceCollection::GetReturnResourcesCallbackForImplThread() {
+  return base::Bind(&UnrefResourcesOnImplThread,
+                    weak_ptr_factory_.GetWeakPtr(),
+                    main_thread_runner_);
 }
 
 }  // namespace cc
diff --git a/cc/layers/delegated_frame_resource_collection.h b/cc/layers/delegated_frame_resource_collection.h
index 68ee11a..9a5d336 100644
--- a/cc/layers/delegated_frame_resource_collection.h
+++ b/cc/layers/delegated_frame_resource_collection.h
@@ -7,6 +7,7 @@
 
 #include "base/containers/hash_tables.h"
 #include "base/memory/ref_counted.h"
+#include "base/memory/weak_ptr.h"
 #include "base/threading/thread_checker.h"
 #include "cc/base/cc_export.h"
 #include "cc/resources/return_callback.h"
@@ -46,10 +47,6 @@ class CC_EXPORT DelegatedFrameResourceCollection
   friend class base::RefCounted<DelegatedFrameResourceCollection>;
   ~DelegatedFrameResourceCollection();
 
-  void UnrefResourcesOnImplThread(
-      scoped_refptr<BlockingTaskRunner> main_thread_runner,
-      const ReturnedResourceArray& returned);
-
   DelegatedFrameResourceCollectionClient* client_;
   scoped_refptr<BlockingTaskRunner> main_thread_runner_;
 
@@ -64,6 +61,7 @@ class CC_EXPORT DelegatedFrameResourceCollection
   ResourceIdRefCountMap resource_id_ref_count_map_;
 
   base::ThreadChecker main_thread_checker_;
+  base::WeakPtrFactory<DelegatedFrameResourceCollection> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(DelegatedFrameResourceCollection);
 };
diff --git a/cc/layers/delegated_frame_resource_collection_unittest.cc b/cc/layers/delegated_frame_resource_collection_unittest.cc
new file mode 100644
index 0000000..ecbc7b1
--- /dev/null
+++ b/cc/layers/delegated_frame_resource_collection_unittest.cc
@@ -0,0 +1,158 @@
+// Copyright 2013 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 "base/bind.h"
+#include "base/run_loop.h"
+#include "base/synchronization/waitable_event.h"
+#include "base/threading/thread.h"
+#include "cc/layers/delegated_frame_resource_collection.h"
+#include "cc/resources/returned_resource.h"
+#include "cc/resources/transferable_resource.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace cc {
+namespace {
+
+class DelegatedFrameResourceCollectionTest
+    : public testing::Test,
+      public DelegatedFrameResourceCollectionClient {
+ protected:
+  DelegatedFrameResourceCollectionTest() : resources_available_(false) {}
+
+  virtual void SetUp() OVERRIDE { CreateResourceCollection(); }
+
+  virtual void TearDown() OVERRIDE { DestroyResourceCollection(); }
+
+  void CreateResourceCollection() {
+    DCHECK(!resource_collection_);
+    resource_collection_ = new DelegatedFrameResourceCollection;
+    resource_collection_->SetClient(this);
+  }
+
+  void DestroyResourceCollection() {
+    if (resource_collection_) {
+      resource_collection_->SetClient(NULL);
+      resource_collection_ = NULL;
+    }
+  }
+
+  TransferableResourceArray CreateResourceArray() {
+    TransferableResourceArray resources;
+    TransferableResource resource;
+    resource.id = 444;
+    resources.push_back(resource);
+    return resources;
+  }
+
+  virtual void UnusedResourcesAreAvailable() OVERRIDE {
+    resources_available_ = true;
+    resource_collection_->TakeUnusedResourcesForChildCompositor(
+        &returned_resources_);
+    if (!resources_available_closure_.is_null())
+      resources_available_closure_.Run();
+  }
+
+  bool ReturnAndResetResourcesAvailable() {
+    bool r = resources_available_;
+    resources_available_ = false;
+    return r;
+  }
+
+  scoped_refptr<DelegatedFrameResourceCollection> resource_collection_;
+  bool resources_available_;
+  ReturnedResourceArray returned_resources_;
+  base::Closure resources_available_closure_;
+};
+
+// This checks that taking the return callback doesn't take extra refcounts,
+// since it's sent to other threads.
+TEST_F(DelegatedFrameResourceCollectionTest, NoRef) {
+  // Start with one ref.
+  EXPECT_TRUE(resource_collection_->HasOneRef());
+
+  ReturnCallback return_callback =
+      resource_collection_->GetReturnResourcesCallbackForImplThread();
+
+  // Callback shouldn't take a ref since it's sent to other threads.
+  EXPECT_TRUE(resource_collection_->HasOneRef());
+}
+
+void ReturnResourcesOnThread(ReturnCallback callback,
+                             const ReturnedResourceArray& resources,
+                             base::WaitableEvent* event) {
+  callback.Run(resources);
+  event->Wait();
+}
+
+// Tests that the ReturnCallback can run safely on threads even after the
+// last references to the collection were dropped.
+TEST_F(DelegatedFrameResourceCollectionTest, Thread) {
+  base::Thread thread("test thread");
+  thread.Start();
+
+  TransferableResourceArray resources = CreateResourceArray();
+  resource_collection_->ReceivedResources(resources);
+  resource_collection_->RefResources(resources);
+
+  ReturnedResourceArray returned_resources;
+  TransferableResource::ReturnResources(resources, &returned_resources);
+
+  base::WaitableEvent event(false, false);
+
+  {
+    base::RunLoop run_loop;
+    resources_available_closure_ = run_loop.QuitClosure();
+
+    thread.message_loop()->PostTask(
+        FROM_HERE,
+        base::Bind(
+            &ReturnResourcesOnThread,
+            resource_collection_->GetReturnResourcesCallbackForImplThread(),
+            returned_resources,
+            &event));
+
+    run_loop.Run();
+  }
+  EXPECT_TRUE(ReturnAndResetResourcesAvailable());
+  EXPECT_EQ(1u, returned_resources_.size());
+  EXPECT_EQ(444u, returned_resources_[0].id);
+  EXPECT_EQ(1, returned_resources_[0].count);
+  returned_resources_.clear();
+
+  // The event prevents the return resources callback from being deleted.
+  // Destroy the last reference from this thread to the collection before
+  // signaling the event, to ensure any reference taken by the callback, if any,
+  // would be the last one.
+  DestroyResourceCollection();
+  event.Signal();
+
+  CreateResourceCollection();
+  resource_collection_->ReceivedResources(resources);
+  resource_collection_->RefResources(resources);
+
+  // Destroy the collection before we have a chance to run the return callback.
+  ReturnCallback return_callback =
+      resource_collection_->GetReturnResourcesCallbackForImplThread();
+  resource_collection_->LoseAllResources();
+  DestroyResourceCollection();
+
+  EXPECT_TRUE(ReturnAndResetResourcesAvailable());
+  EXPECT_EQ(1u, returned_resources_.size());
+  EXPECT_EQ(444u, returned_resources_[0].id);
+  EXPECT_EQ(1, returned_resources_[0].count);
+  EXPECT_TRUE(returned_resources_[0].lost);
+  returned_resources_.clear();
+
+  thread.message_loop()->PostTask(FROM_HERE,
+                                  base::Bind(&ReturnResourcesOnThread,
+                                             return_callback,
+                                             returned_resources,
+                                             &event));
+  event.Signal();
+
+  thread.Stop();
+}
+
+}  // namespace
+}  // namespace cc