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..59901e208ce54db2c9f60f3eb363e36ea33f9a48 100644
--- a/components/webdata/common/web_data_request_manager.cc
+++ b/components/webdata/common/web_data_request_manager.cc
@@ -18,12 +18,13 @@
 //
 ////////////////////////////////////////////////////////////////////////////////
 
-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(WebDataRequestManager* manager,

[Peter Kasting, 2016/12/15 01:05:34]

Nit: While here, maybe fix function definition order to match declaration order.

[Roger McFarlane (Chromium), 2016/12/15 07:41:45]

Done.

+                               WebDataServiceConsumer* consumer)
+    : task_runner_(base::ThreadTaskRunnerHandle::Get()),
+      manager_(manager),
+      consumer_(consumer),
+      handle_(0) {
+  DCHECK(manager_ != nullptr);

[Peter Kasting, 2016/12/15 01:05:34]

Nit: DCHECK(IsActive())?

[Roger McFarlane (Chromium), 2016/12/15 07:41:45]

Done.

 }
 
 WebDataRequest::~WebDataRequest() {
@@ -36,7 +37,20 @@ WebDataServiceBase::Handle WebDataRequest::GetHandle() const {
   return handle_;
 }
 
+bool WebDataRequest::IsActive() const {
+  return manager_ != nullptr;
+}
+
+inline void WebDataRequest::AssertThreadSafe() const {

[Peter Kasting, 2016/12/15 01:05:34]

Nit: Remove "inline" (doesn't match class declaration, not really necessary
anyway)

[Roger McFarlane (Chromium), 2016/12/15 07:41:45]

Done.

+  // Manipulations of the request state should only happen when the request is
+  // active (i.e., the manager is non-null) and the manager's lock is held

[Peter Kasting, 2016/12/15 01:05:34]

Nit: Remove "(i.e., the manager is non-null)" and change the DCHECK below to
check IsActive() directly.

[Roger McFarlane (Chromium), 2016/12/15 07:41:45]

Done.

+  // (i.e., during request cancellation or completion).
+  DCHECK(manager_ != nullptr);
+  manager_->AssertLockedByCurrentThread();
+}
+
 WebDataServiceConsumer* WebDataRequest::GetConsumer() const {
+  AssertThreadSafe();
   return consumer_;
 }
 
@@ -45,28 +59,10 @@ scoped_refptr<base::SingleThreadTaskRunner> WebDataRequest::GetTaskRunner()
   return task_runner_;
 }
 
-bool WebDataRequest::IsCancelled() const {
-  base::AutoLock l(cancel_lock_);
-  return cancelled_;
-}
-
-void WebDataRequest::Cancel() {
-  base::AutoLock l(cancel_lock_);
-  cancelled_ = true;
-  consumer_ = NULL;
-  manager_ = NULL;
-}
-
-void WebDataRequest::OnComplete() {
-  manager_= NULL;
-}
-
-void WebDataRequest::SetResult(std::unique_ptr<WDTypedResult> r) {
-  result_ = std::move(r);
-}
-
-std::unique_ptr<WDTypedResult> WebDataRequest::GetResult() {
-  return std::move(result_);
+void WebDataRequest::MarkAsInactive() {
+  AssertThreadSafe();
+  consumer_ = nullptr;
+  manager_ = nullptr;
 }
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -82,71 +78,98 @@ WebDataRequestManager::WebDataRequestManager()
 WebDataRequestManager::~WebDataRequestManager() {
   base::AutoLock l(pending_lock_);
   for (auto i = pending_requests_.begin(); i != pending_requests_.end(); ++i)
-    i->second->Cancel();
+    i->second->MarkAsInactive();
   pending_requests_.clear();
 }
 
-void WebDataRequestManager::RegisterRequest(WebDataRequest* request) {
-  base::AutoLock l(pending_lock_);
-  pending_requests_[request->GetHandle()] = request;
-}
+std::unique_ptr<WebDataRequest> WebDataRequestManager::NewRequest(
+    WebDataServiceConsumer* consumer) {
+  std::unique_ptr<WebDataRequest> request(new WebDataRequest(this, consumer));
 
-int WebDataRequestManager::GetNextRequestHandle() {
+  // Grab the lock to get the next request handle and register the request.
   base::AutoLock l(pending_lock_);
-  return ++next_request_handle_;
+  request->handle_ = ++next_request_handle_;

[Peter Kasting, 2016/12/15 01:05:34]

Nit: I think this should be next_request_handle_++.  That way, the name is
actually truthful: next_request_handle_ is the next handle that will be given
out, not the most recent one to have been given out.

[Roger McFarlane (Chromium), 2016/12/15 07:41:45]

Done.

+  pending_requests_[request->handle_] = request.get();
+
+  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::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"));
+  // Placeholders for the request consumer, handle and result... to be used

[Peter Kasting, 2016/12/15 01:05:34]

Nit: Comment seems inaccurate?  I don't see the other such placeholders.

I would just nuke the comment and the initializer.

[Roger McFarlane (Chromium), 2016/12/15 07:41:45]

Oops, out of date comment.

I'd prefer to leave a comment to the next reader. I've updated it to better
reflect the why. WDYT?

+  // when notifying the consumer that the request has completed.
+  WebDataServiceConsumer* consumer = nullptr;
+
+  // 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;
+
+    // TODO(robliao): Remove ScopedTracker below once https://crbug.com/422460
+    // is fixed.
+    tracked_objects::ScopedTracker tracking_profile1(

[Peter Kasting, 2016/12/15 01:05:34]

Nit: Rename this variable and the one below to no longer have numeric suffixes,
since they are no longer in the same scope.

[Roger McFarlane (Chromium), 2016/12/15 07:41:45]

Done.

+        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"));
+    // Remember the consumer and result for notification below.
+    consumer = request->GetConsumer();
 
-  // Notify the consumer if needed.
-  if (!request->IsCancelled()) {
-    WebDataServiceConsumer* consumer = request->GetConsumer();
-    request->OnComplete();
-    if (consumer) {
-      consumer->OnWebDataServiceRequestDone(request->GetHandle(),
-                                            request->GetResult());
-    }
+    // 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.
+  if (consumer) {
+    // 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"));
+
+    consumer->OnWebDataServiceRequestDone(request->GetHandle(),
+                                          std::move(result));
+  }
 }