Remove data race in WebDataRequest::CancelRequest.

components/webdata/common/web_data_request_manager.cc

 // Copyright 2012 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 "components/webdata/common/web_data_request_manager.h"
 
 #include <utility>
 
 #include "base/bind.h"
 #include "base/location.h"
 #include "base/profiler/scoped_tracker.h"
 #include "base/stl_util.h"
 #include "base/threading/thread_task_runner_handle.h"
 
 ////////////////////////////////////////////////////////////////////////////////
 //
 // WebDataRequest implementation.
 //
 ////////////////////////////////////////////////////////////////////////////////
 
 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;
+  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_;
 }
 
 scoped_refptr<base::SingleThreadTaskRunner> WebDataRequest::GetTaskRunner()
     const {
   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);
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 //
 // WebDataRequestManager implementation.
 //
 ////////////////////////////////////////////////////////////////////////////////
 
 WebDataRequestManager::WebDataRequestManager()
     : next_request_handle_(1) {
 }
 
 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;
 }
 
 void WebDataRequestManager::CancelRequest(WebDataServiceBase::Handle h) {
   base::AutoLock l(pending_lock_);
   // If the request was already cancelled, or has already completed, it won't
   // be in the pending_requests_ collection any more.
   auto i = pending_requests_.find(h);
   if (i == pending_requests_.end())
     return;
   i->second->MarkAsInactive();
   pending_requests_.erase(i);
 }
 
 void WebDataRequestManager::RequestCompleted(
     std::unique_ptr<WebDataRequest> request,
     std::unique_ptr<WDTypedResult> result) {
   scoped_refptr<base::SingleThreadTaskRunner> task_runner =
       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() {
   base::AutoLock l(pending_lock_);
   for (auto i = pending_requests_.begin(); i != pending_requests_.end(); ++i)
     i->second->MarkAsInactive();
   pending_requests_.clear();
 }
 
 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
     // another thread before the lock was acquired.

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

This comment seems misleading (we're no longer "re-"checking anything, since the
first check was removed).

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

Done.

     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(
         FROM_HERE_WITH_EXPLICIT_FUNCTION(
             "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.
     request->MarkAsInactive();
   }
 
   // Notify the consumer if needed.
   //
   // 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.
     tracked_objects::ScopedTracker tracking_profile(
         FROM_HERE_WITH_EXPLICIT_FUNCTION(
             "422460 "
             "WebDataRequestManager::RequestCompletedOnThread::NotifyConsumer"));
 
     consumer->OnWebDataServiceRequestDone(request->GetHandle(),
                                           std::move(result));
   }
 }