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..41cc0dfaeed8a993b8f9010c4adf517dde857d0c 100644
--- a/components/webdata/common/web_data_request_manager.cc
+++ b/components/webdata/common/web_data_request_manager.cc
@@ -19,9 +19,9 @@
 ////////////////////////////////////////////////////////////////////////////////
 
 WebDataRequest::~WebDataRequest() {
-  if (IsActive()) {
-    manager_->CancelRequest(handle_);
-  }
+  WebDataRequestManager* manager = GetManager();
+  if (manager)
+    manager->CancelRequest(handle_);
 }
 
 WebDataServiceBase::Handle WebDataRequest::GetHandle() const {
@@ -29,28 +29,26 @@ WebDataServiceBase::Handle WebDataRequest::GetHandle() const {
 }
 
 bool WebDataRequest::IsActive() const {
-  return manager_ != nullptr;
+  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());
 }
 
-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() const {
+  return reinterpret_cast<WebDataRequestManager*>(
+      base::subtle::Acquire_Load(&atomic_manager_));
 }
 
 WebDataServiceConsumer* WebDataRequest::GetConsumer() const {
-  AssertThreadSafe();
   return consumer_;
 }
 
@@ -60,9 +58,8 @@ scoped_refptr<base::SingleThreadTaskRunner> WebDataRequest::GetTaskRunner()
 }
 
 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 +74,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(
+      new WebDataRequest(this, consumer, next_request_handle_));

[Peter Kasting, 2017/04/28 06:36:41]

Nit: Prefer = MakeUnique to bare new

[Roger McFarlane (Chromium), 2017/04/28 15:06:13]

That doesn't play nicely with non-public constructors. I wasn't able to find any
occurrences of friend base::MakeUnique. Is it better to make the constructor
public?

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

Mmm.  I didn't think through the constructor being non-public.  Probably better
to just leave it as you had it, then.

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

Acknowledged.

+  bool inserted =
+      pending_requests_.emplace(next_request_handle_, request.get()).second;
+  DCHECK(inserted);
+  ++next_request_handle_;
   return request;
 }
 
@@ -105,12 +102,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,16 +116,6 @@ 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_);
@@ -142,9 +125,6 @@ void WebDataRequestManager::RequestCompletedOnThread(
     if (!request->IsActive())
       return;
 
-    // Remember the consumer for notification below.
-    consumer = request->GetConsumer();

[Peter Kasting, 2017/04/28 06:36:41]

This isn't safe to remove.  MarkAsInactive() nukes the |consumer_| on the
request, which you'll need afterwards.

[Roger McFarlane (Chromium), 2017/04/28 15:06:13]

consumer_ doesn't gets nuked anymore (I made it const).

-
     // TODO(robliao): Remove ScopedTracker below once https://crbug.com/422460
     // is fixed.
     tracked_objects::ScopedTracker tracking_profile(
@@ -152,10 +132,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());

[Roger McFarlane (Chromium), 2017/04/27 20:51:44]

The code in this block is just CancelRequest() + a ScoperTracker.

This could equivalently by:

void WebDataRequestManager::RequestCompletedOnThread(
    std::unique_ptr<WebDataRequest> request,
    std::unique_ptr<WDTypedResult> result) {
  if (!request->IsActive())
    return;

  {
    // This tracker could be moved into CancelRequest?
    tracked_objects::ScopedTracker tracking_profile(
        FROM_HERE_WITH_EXPLICIT_FUNCTION("..."));
    CancelRequest(request->GetHandle());
  }

  auto* const consumer = request->GetConsumer();
  if (consumer) {
    tracked_objects::ScopedTracker tracking_profile(
        FROM_HERE_WITH_EXPLICIT_FUNCTION("..."));
    consumer->OnWebDataServiceRequestDone(request->GetHandle());
  }
}

[Peter Kasting, 2017/04/28 06:36:41]

Doing this will require some refactoring given the bit about needing to save off
the consumer.

[Roger McFarlane (Chromium), 2017/04/28 15:06:13]

consumer_ no longer gets set to nullptr, only the active/inactive state is
toggled by MarkAsInactive.

one could also grab the consumer before hand.

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

In that case, the change you suggest seems like a good one.  Could happen in
this CL or a separate one.

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

i'll send another CL.

-
-    // Take ownership of the request object and remove it from the map.
     pending_requests_.erase(i);
 
     // The request is no longer active.
@@ -166,6 +146,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.