Remove data race in WebDataRequest::CancelRequest.

components/webdata/common/web_data_request_manager.cc

Index: components/webdata/common/web_data_request_manager.cc
diff --git a/components/webdata/common/web_data_request_manager.cc b/components/webdata/common/web_data_request_manager.cc
index 3615379a7584f081735fa26a7b51884a2d98bae4..ec272c06a940cbe121c19a48c1dc7c5b9ffbb1a3 100644
--- a/components/webdata/common/web_data_request_manager.cc
+++ b/components/webdata/common/web_data_request_manager.cc
@@ -8,6 +8,7 @@
 
 #include "base/bind.h"
 #include "base/location.h"
+#include "base/memory/ptr_util.h"
 #include "base/profiler/scoped_tracker.h"
 #include "base/stl_util.h"
 #include "base/threading/thread_task_runner_handle.h"
@@ -19,50 +20,47 @@
 ////////////////////////////////////////////////////////////////////////////////
 
 WebDataRequest::~WebDataRequest() {
-  if (IsActive()) {
-    manager_->CancelRequest(handle_);
-  }
+  WebDataRequestManager* manager = GetManager();
+  if (manager)
+    manager->CancelRequest(handle_);
 }
 
 WebDataServiceBase::Handle WebDataRequest::GetHandle() const {
   return handle_;
 }
 
-bool WebDataRequest::IsActive() const {
-  return manager_ != nullptr;
+bool WebDataRequest::IsActive() {

[Peter Kasting, 2017/05/04 18:32:25]

Losing the "const" here seems sad, but inlining the complexity of GetManager()
or adding a parallel, const version of that function also seem kinda sad.  Maybe
this is the easiest solution.

[Roger McFarlane (Chromium), 2017/05/05 18:58:49]

Acknowledged.

+  return GetManager() != nullptr;
 }
 
 WebDataRequest::WebDataRequest(WebDataRequestManager* manager,
-                               WebDataServiceConsumer* consumer)
+                               WebDataServiceConsumer* consumer,
+                               WebDataServiceBase::Handle handle)
     : task_runner_(base::ThreadTaskRunnerHandle::Get()),
-      manager_(manager),
+      atomic_manager_(reinterpret_cast<base::subtle::AtomicWord>(manager)),
       consumer_(consumer),
-      handle_(0) {
+      handle_(handle) {
+  DCHECK(task_runner_);
   DCHECK(IsActive());
+  static_assert(sizeof(atomic_manager_) == sizeof(manager), "size mismatch");
 }
 
-void WebDataRequest::AssertThreadSafe() const {
-  // Manipulations of the request state should only happen when the request is
-  // active and the manager's lock is held (i.e., during request cancellation
-  // or completion).
-  DCHECK(IsActive());
-  manager_->AssertLockedByCurrentThread();
+WebDataRequestManager* WebDataRequest::GetManager() {
+  return reinterpret_cast<WebDataRequestManager*>(
+      base::subtle::Acquire_Load(&atomic_manager_));
 }
 
-WebDataServiceConsumer* WebDataRequest::GetConsumer() const {
-  AssertThreadSafe();
+WebDataServiceConsumer* WebDataRequest::GetConsumer() {
   return consumer_;
 }
 
-scoped_refptr<base::SingleThreadTaskRunner> WebDataRequest::GetTaskRunner()
-    const {
+scoped_refptr<base::SingleThreadTaskRunner> WebDataRequest::GetTaskRunner() {
   return task_runner_;
 }
 
 void WebDataRequest::MarkAsInactive() {
-  AssertThreadSafe();
-  consumer_ = nullptr;
-  manager_ = nullptr;
+  // Set atomic_manager_ to the equivalent of nullptr;
+  base::subtle::Release_Store(&atomic_manager_, 0);
 }
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -77,13 +75,13 @@ WebDataRequestManager::WebDataRequestManager()
 
 std::unique_ptr<WebDataRequest> WebDataRequestManager::NewRequest(
     WebDataServiceConsumer* consumer) {
-  std::unique_ptr<WebDataRequest> request(new WebDataRequest(this, consumer));
-
-  // Grab the lock to get the next request handle and register the request.
   base::AutoLock l(pending_lock_);
-  request->handle_ = next_request_handle_++;
-  pending_requests_[request->handle_] = request.get();
-
+  std::unique_ptr<WebDataRequest> request = base::WrapUnique(
+      new WebDataRequest(this, consumer, next_request_handle_));
+  bool inserted =
+      pending_requests_.emplace(next_request_handle_, request.get()).second;
+  DCHECK(inserted);
+  ++next_request_handle_;
   return request;
 }
 
@@ -105,12 +103,8 @@ void WebDataRequestManager::RequestCompleted(
       request->GetTaskRunner();
   task_runner->PostTask(
       FROM_HERE,
-      base::Bind(&WebDataRequestManager::RequestCompletedOnThread, this,
-                 base::Passed(&request), base::Passed(&result)));
-}
-
-void WebDataRequestManager::AssertLockedByCurrentThread() const {
-  pending_lock_.AssertAcquired();
+      base::BindOnce(&WebDataRequestManager::RequestCompletedOnThread, this,
+                     base::Passed(&request), base::Passed(&result)));
 }
 
 WebDataRequestManager::~WebDataRequestManager() {
@@ -123,28 +117,15 @@ WebDataRequestManager::~WebDataRequestManager() {
 void WebDataRequestManager::RequestCompletedOnThread(
     std::unique_ptr<WebDataRequest> request,
     std::unique_ptr<WDTypedResult> result) {
-  // If the request is already inactive, early exit to avoid contending for the
-  // lock (aka, the double checked locking pattern).
-  if (!request->IsActive())
-    return;
-
-  // Used to export the consumer from the locked region below so that the
-  // consumer can be notified outside of the lock and after the request has
-  // been marked as inactive.
-  WebDataServiceConsumer* consumer;
-
   // Manipulate the pending_requests_ collection while holding the lock.
   {
     base::AutoLock l(pending_lock_);
 
-    // Re-check whether the request is active. It might have been cancelled in
+    // Check whether the request is active. It might have been cancelled in
     // another thread before the lock was acquired.
     if (!request->IsActive())
       return;
 
-    // Remember the consumer for notification below.
-    consumer = request->GetConsumer();
-
     // TODO(robliao): Remove ScopedTracker below once https://crbug.com/422460
     // is fixed.
     tracked_objects::ScopedTracker tracking_profile(
@@ -152,10 +133,10 @@ void WebDataRequestManager::RequestCompletedOnThread(
             "422460 "
             "WebDataRequestManager::RequestCompletedOnThread::UpdateMap"));
 
+    // Remove the request object from the pending_requests_ map. Note that this
+    // method has ownership of the object (it was passed by unique_ptr).
     auto i = pending_requests_.find(request->GetHandle());
     DCHECK(i != pending_requests_.end());
-
-    // Take ownership of the request object and remove it from the map.
     pending_requests_.erase(i);
 
     // The request is no longer active.
@@ -166,6 +147,7 @@ void WebDataRequestManager::RequestCompletedOnThread(
   //
   // NOTE: The pending_lock_ is no longer held here. It's up to the consumer to
   // be appropriately thread safe.
+  WebDataServiceConsumer* const consumer = request->GetConsumer();
   if (consumer) {
     // TODO(robliao): Remove ScopedTracker below once https://crbug.com/422460
     // is fixed.