Fix threading issues in WebDataRequestManager.

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 d8a271d4a8b26f9bb1c3135127e37d17ea3c3adb..3587d8fa4697cd90e424f483061d3957eb7649c7 100644
--- a/components/webdata/common/web_data_request_manager.cc
+++ b/components/webdata/common/web_data_request_manager.cc
@@ -18,16 +18,8 @@
 //
 ////////////////////////////////////////////////////////////////////////////////
 
-WebDataRequest::WebDataRequest(WebDataServiceConsumer* consumer,
-                               WebDataRequestManager* manager)
-    : manager_(manager), cancelled_(false), consumer_(consumer) {
-  handle_ = manager_->GetNextRequestHandle();
-  task_runner_ = base::ThreadTaskRunnerHandle::Get();
-  manager_->RegisterRequest(this);
-}
-
 WebDataRequest::~WebDataRequest() {
-  if (manager_) {
+  if (IsActive()) {
     manager_->CancelRequest(handle_);
   }
 }
@@ -36,37 +28,41 @@ WebDataServiceBase::Handle WebDataRequest::GetHandle() const {
   return handle_;
 }
 
-WebDataServiceConsumer* WebDataRequest::GetConsumer() const {
-  return consumer_;
-}
-
-scoped_refptr<base::SingleThreadTaskRunner> WebDataRequest::GetTaskRunner()
-    const {
-  return task_runner_;
+bool WebDataRequest::IsActive() const {
+  return manager_ != nullptr;
 }
 
-bool WebDataRequest::IsCancelled() const {
-  base::AutoLock l(cancel_lock_);
-  return cancelled_;
+WebDataRequest::WebDataRequest(WebDataRequestManager* manager,
+                               WebDataServiceConsumer* consumer)
+    : task_runner_(base::ThreadTaskRunnerHandle::Get()),
+      manager_(manager),
+      consumer_(consumer),
+      handle_(0) {
+  DCHECK(IsActive());
 }
 
-void WebDataRequest::Cancel() {
-  base::AutoLock l(cancel_lock_);
-  cancelled_ = true;
-  consumer_ = NULL;
-  manager_ = NULL;
+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();
 }
 
-void WebDataRequest::OnComplete() {
-  manager_= NULL;
+WebDataServiceConsumer* WebDataRequest::GetConsumer() const {
+  AssertThreadSafe();
+  return consumer_;
 }
 
-void WebDataRequest::SetResult(std::unique_ptr<WDTypedResult> r) {
-  result_ = std::move(r);
+scoped_refptr<base::SingleThreadTaskRunner> WebDataRequest::GetTaskRunner()
+    const {
+  return task_runner_;
 }
 
-std::unique_ptr<WDTypedResult> WebDataRequest::GetResult() {
-  return std::move(result_);
+void WebDataRequest::MarkAsInactive() {
+  AssertThreadSafe();
+  consumer_ = nullptr;
+  manager_ = nullptr;
 }
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -79,74 +75,106 @@ WebDataRequestManager::WebDataRequestManager()
     : next_request_handle_(1) {
 }
 
-WebDataRequestManager::~WebDataRequestManager() {
-  base::AutoLock l(pending_lock_);
-  for (auto i = pending_requests_.begin(); i != pending_requests_.end(); ++i)
-    i->second->Cancel();
-  pending_requests_.clear();
-}
+std::unique_ptr<WebDataRequest> WebDataRequestManager::NewRequest(
+    WebDataServiceConsumer* consumer) {
+  std::unique_ptr<WebDataRequest> request(new WebDataRequest(this, consumer));
 
-void WebDataRequestManager::RegisterRequest(WebDataRequest* request) {
+  // Grab the lock to get the next request handle and register the request.
   base::AutoLock l(pending_lock_);
-  pending_requests_[request->GetHandle()] = request;
-}
+  request->handle_ = next_request_handle_++;
+  pending_requests_[request->handle_] = request.get();
 
-int WebDataRequestManager::GetNextRequestHandle() {
-  base::AutoLock l(pending_lock_);
-  return ++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);
-  DCHECK(i != pending_requests_.end());
-  i->second->Cancel();
+  if (i == pending_requests_.end())
+    return;
+  i->second->MarkAsInactive();
   pending_requests_.erase(i);
 }
 
 void WebDataRequestManager::RequestCompleted(
-    std::unique_ptr<WebDataRequest> request) {
+    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)));
+      FROM_HERE,
+      base::Bind(&WebDataRequestManager::RequestCompletedOnThread, this,
+                 base::Passed(&request), base::Passed(&result)));
+}
+
+void WebDataRequestManager::AssertLockedByCurrentThread() const {
+  pending_lock_.AssertAcquired();
+}
+
+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) {
-  if (request->IsCancelled())
+    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;
 
-  // TODO(robliao): Remove ScopedTracker below once https://crbug.com/422460 is
-  // fixed.
-  tracked_objects::ScopedTracker tracking_profile1(
-      FROM_HERE_WITH_EXPLICIT_FUNCTION(
-          "422460 WebDataRequestManager::RequestCompletedOnThread::UpdateMap"));
+  // Used to export the consumer from the locked region below so that consumer

[Peter Kasting, 2016/12/15 07:50:17]

Yeah, this comment seems fine.

Nit: I'd say "so that the" instead of "so that".

[Roger McFarlane (Chromium), 2016/12/15 16:30:47]

Done.

+  // 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.
+    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(
+        FROM_HERE_WITH_EXPLICIT_FUNCTION(
+            "422460 "
+            "WebDataRequestManager::RequestCompletedOnThread::UpdateMap"));
+
     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);
-  }
 
-  // TODO(robliao): Remove ScopedTracker below once https://crbug.com/422460 is
-  // fixed.
-  tracked_objects::ScopedTracker tracking_profile2(
-      FROM_HERE_WITH_EXPLICIT_FUNCTION(
-          "422460 "
-          "WebDataRequestManager::RequestCompletedOnThread::NotifyConsumer"));
+    // The request is no longer active.
+    request->MarkAsInactive();
+  }
 
   // Notify the consumer if needed.
-  if (!request->IsCancelled()) {
-    WebDataServiceConsumer* consumer = request->GetConsumer();
-    request->OnComplete();
-    if (consumer) {
-      consumer->OnWebDataServiceRequestDone(request->GetHandle(),
-                                            request->GetResult());
-    }
+  //
+  // NOTE: The pending_lock_ is no longer held here. It's up to the consumer to
+  // be appropriately thread safe.
+  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));
   }
-
 }