Stop binding ownerless ref-count objects in //content

An upcoming change to base::Bind rejects binding ref counted objects
when their ref count is zero, as that pattern of usage is error prone.
This CL updates that pattern in //content.

For DOMStorageContextWrapper, the instance and a MemoryPressureListener
used to own each other implicitly. And, after this CL,
DOMStorageContextWrapper owns MemoryPressureListener.

For PlatformNotificationContextImpl, its Initialize() makes a reference
to the instance, and drops it soon. So, without keeping a reference
in caller, Initialize() may destroy the instance.

For WebContentsImpl::RequestAXTreeSnapshot, this should also keep a
reference to the instance, as AXTreeSnapshotCombiner::AddFrame called
by RecursiveRequestAXTreeSnapshotOnFrame makes a reference to
AXTreeSnapshotCombiner, and releases the reference after the method
call. So, that destroys AXTreeSnapshotCombiner instance implicitly.

SW's StoppedObserever and TestFileErrorInjector is the false positive
cases of the check.

Bug: 866456
Change-Id: Ie99c996fdcf2413b170de2a246d63c34433a24ac
Reviewed-on: https://chromium-review.googlesource.com/1156200Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Commit-Queue: Taiju Tsuiki <tzik@chromium.org>
Cr-Commit-Position: refs/heads/master@{#579754}

content/browser/dom_storage/dom_storage_context_wrapper.cc

@@ -173,7 +173,8 @@ DOMStorageContextWrapper::DOMStorageContextWrapper(
     base::MemoryCoordinatorClientRegistry::GetInstance()->Register(this);
   } else {
     memory_pressure_listener_.reset(new base::MemoryPressureListener(
-        base::Bind(&DOMStorageContextWrapper::OnMemoryPressure, this)));
+        base::BindRepeating(&DOMStorageContextWrapper::OnMemoryPressure,
+                            base::Unretained(this))));
   }
 }
 

content/browser/notifications/platform_notification_context_unittest.cc

@@ -101,13 +101,13 @@ class PlatformNotificationContextTest : public ::testing::Test {
  protected:
   // Creates a new PlatformNotificationContextImpl instance. When using this
   // method, the underlying database will always be created in memory.
-  PlatformNotificationContextImpl* CreatePlatformNotificationContext() {
-    PlatformNotificationContextImpl* context =
-        new PlatformNotificationContextImpl(base::FilePath(), &browser_context_,
-                                            nullptr);
+  scoped_refptr<PlatformNotificationContextImpl>
+  CreatePlatformNotificationContext() {
+    auto context = base::MakeRefCounted<PlatformNotificationContextImpl>(
+        base::FilePath(), &browser_context_, nullptr);
     context->Initialize();
 
-    OverrideTaskRunnerForTesting(context);
+    OverrideTaskRunnerForTesting(context.get());
     return context;
   }
 

content/browser/web_contents/web_contents_impl.cc

@@ -1121,10 +1121,10 @@ void WebContentsImpl::RequestAXTreeSnapshot(AXTreeSnapshotCallback callback,
   // them into a single tree and call |callback| with that result, then
   // delete |combiner|.
   FrameTreeNode* root_node = frame_tree_.root();
-  AXTreeSnapshotCombiner* combiner =
-      new AXTreeSnapshotCombiner(std::move(callback));
+  auto combiner =
+      base::MakeRefCounted<AXTreeSnapshotCombiner>(std::move(callback));
 
-  RecursiveRequestAXTreeSnapshotOnFrame(root_node, combiner, ax_mode);
+  RecursiveRequestAXTreeSnapshotOnFrame(root_node, combiner.get(), ax_mode);
 }
 
 void WebContentsImpl::RecursiveRequestAXTreeSnapshotOnFrame(

content/public/test/service_worker_test_helpers.cc

@@ -17,19 +17,25 @@ namespace {
 
 class StoppedObserver : public base::RefCountedThreadSafe<StoppedObserver> {
  public:
-  StoppedObserver(ServiceWorkerContextWrapper* context,
-                  int64_t service_worker_version_id,
-                  base::OnceClosure completion_callback_ui)
-      : inner_observer_(context,
-                        service_worker_version_id,
-                        // Adds a ref to StoppedObserver to keep |this| around
-                        // until the worker is stopped.
-                        base::BindOnce(&StoppedObserver::OnStopped, this)),
-        completion_callback_ui_(std::move(completion_callback_ui)) {}
+  static void StartObserving(ServiceWorkerContextWrapper* context,
+                             int64_t service_worker_version_id,
+                             base::OnceClosure completion_callback_ui) {
+    auto observer = base::WrapRefCounted(
+        new StoppedObserver(std::move(completion_callback_ui)));
+    // Adds a ref to StoppedObserver to keep |this| around until the worker is
+    // stopped.
+    observer->inner_observer_ = std::make_unique<Observer>(
+        context, service_worker_version_id,
+        base::BindOnce(&StoppedObserver::OnStopped, observer));
+  }
 
  private:
   friend class base::RefCountedThreadSafe<StoppedObserver>;
+
+  explicit StoppedObserver(base::OnceClosure completion_callback_ui)
+      : completion_callback_ui_(std::move(completion_callback_ui)) {}
   ~StoppedObserver() {}
+
   class Observer : public ServiceWorkerContextCoreObserver {
    public:
     Observer(ServiceWorkerContextWrapper* context,
@@ -67,7 +73,7 @@ class StoppedObserver : public base::RefCountedThreadSafe<StoppedObserver> {
     std::move(completion_callback_ui_).Run();
   }
 
-  Observer inner_observer_;
+  std::unique_ptr<Observer> inner_observer_;
   base::OnceClosure completion_callback_ui_;
 
   DISALLOW_COPY_AND_ASSIGN(StoppedObserver);
@@ -81,8 +87,8 @@ void FoundReadyRegistration(
   DCHECK_EQ(blink::ServiceWorkerStatusCode::kOk, service_worker_status);
   int64_t version_id =
       service_worker_registration->active_version()->version_id();
-  scoped_refptr<StoppedObserver> observer(new StoppedObserver(
-      context_wrapper, version_id, std::move(completion_callback)));
+  StoppedObserver::StartObserving(context_wrapper, version_id,
+                                  std::move(completion_callback));
   service_worker_registration->active_version()->embedded_worker()->Stop();
 }
 

content/public/test/test_file_error_injector.cc

@@ -350,17 +350,6 @@ TestFileErrorInjector::TestFileErrorInjector(DownloadManager* download_manager)
     :  // This code is only used for browser_tests, so a
       // DownloadManager is always a DownloadManagerImpl.
       download_manager_(static_cast<DownloadManagerImpl*>(download_manager)) {
-  // Record the value of the pointer, for later validation.
-  created_factory_ = new DownloadFileWithErrorFactory(
-      base::Bind(&TestFileErrorInjector::RecordDownloadFileConstruction, this),
-      base::Bind(&TestFileErrorInjector::RecordDownloadFileDestruction, this));
-
-  // We will transfer ownership of the factory to the download manager.
-  std::unique_ptr<download::DownloadFileFactory> download_file_factory(
-      created_factory_);
-
-  download_manager_->SetDownloadFileFactoryForTesting(
-      std::move(download_file_factory));
 }
 
 TestFileErrorInjector::~TestFileErrorInjector() {
@@ -430,6 +419,20 @@ scoped_refptr<TestFileErrorInjector> TestFileErrorInjector::Create(
 
   scoped_refptr<TestFileErrorInjector> single_injector(
       new TestFileErrorInjector(download_manager));
+  // Record the value of the pointer, for later validation.
+  single_injector->created_factory_ = new DownloadFileWithErrorFactory(
+      base::BindRepeating(
+          &TestFileErrorInjector::RecordDownloadFileConstruction,
+          single_injector),
+      base::BindRepeating(&TestFileErrorInjector::RecordDownloadFileDestruction,
+                          single_injector));
+
+  // We will transfer ownership of the factory to the download manager.
+  std::unique_ptr<download::DownloadFileFactory> download_file_factory(
+      single_injector->created_factory_);
+
+  single_injector->download_manager_->SetDownloadFileFactoryForTesting(
+      std::move(download_file_factory));
 
   return single_injector;
 }