Fix threading issues in WebDataRequestManager.

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(WebDataServiceConsumer* consumer,
                                WebDataRequestManager* manager)
-    : manager_(manager), cancelled_(false), consumer_(consumer) {
-  handle_ = manager_->GetNextRequestHandle();
-  task_runner_ = base::ThreadTaskRunnerHandle::Get();
+    : task_runner_(base::ThreadTaskRunnerHandle::Get()),
+      handle_(manager->GetNextRequestHandle()),
+      manager_(manager),
+      consumer_(consumer) {
   manager_->RegisterRequest(this);
 }
 
 WebDataRequest::~WebDataRequest() {
   if (manager_) {
     manager_->CancelRequest(handle_);
   }
 }
 
 WebDataServiceBase::Handle WebDataRequest::GetHandle() const {
   return handle_;
 }
 
+bool WebDataRequest::IsCancelled() const {

[Mathieu, 2016/12/14 01:13:46]

IsInactive? Could be completed at this point too if I read correctly

[Roger McFarlane (Chromium), 2016/12/14 14:02:49]

Done. But used IsActive instead, to avoid the double negative.

+  return manager_ == nullptr;
+}
+
+inline void WebDataRequest::AssertThreadSafe() const {
+  // Of manager_ is null then the request has already been cancelled or

[Mathieu, 2016/12/14 01:13:46]

If

[Roger McFarlane (Chromium), 2016/12/14 14:02:49]

Done.

+  // completed, and the asserting context represents and invalid state.

[Mathieu, 2016/12/14 01:13:46]

*an

[Roger McFarlane (Chromium), 2016/12/14 14:02:49]

Done.

+  DCHECK(manager_);
+
+  // Manipulations of the request state should only happen when the manager's
+  // lock is held (i.e., during request cancellation or completion).
+  manager_->AssertLockedByCurrentThread();
+}
+
 WebDataServiceConsumer* WebDataRequest::GetConsumer() const {
+  AssertThreadSafe();
   return consumer_;
 }
 
 scoped_refptr<base::SingleThreadTaskRunner> WebDataRequest::GetTaskRunner()
     const {
   return task_runner_;
 }
 
-bool WebDataRequest::IsCancelled() const {
-  base::AutoLock l(cancel_lock_);

[Roger McFarlane (Chromium), 2016/12/13 22:33:55]

this lock isn't useful, it's not protecting against anything, either way, the
caller either sees the request as cancelled or not.

-  return cancelled_;
-}
-
-void WebDataRequest::Cancel() {
-  base::AutoLock l(cancel_lock_);

[Roger McFarlane (Chromium), 2016/12/13 22:33:55]

this lock isn't useful.

This code is called under the manager's lock, so the lock is redundant.

-  cancelled_ = true;
-  consumer_ = NULL;
-  manager_ = NULL;
-}
-
-void WebDataRequest::OnComplete() {
-  manager_= NULL;
+void WebDataRequest::MarkAsInactive() {
+  AssertThreadSafe();
+  consumer_ = nullptr;
+  manager_ = nullptr;
 }
 
 void WebDataRequest::SetResult(std::unique_ptr<WDTypedResult> r) {
+  AssertThreadSafe();
   result_ = std::move(r);

[Roger McFarlane (Chromium), 2016/12/13 22:33:55]

this isn't thread safe, could leak r.

 }
 
 std::unique_ptr<WDTypedResult> WebDataRequest::GetResult() {
+  AssertThreadSafe();
   return std::move(result_);

[Roger McFarlane (Chromium), 2016/12/13 22:33:55]

this isn't thread safe, you could end up with multiple unique_ptrs to result_

 }
 
 ////////////////////////////////////////////////////////////////////////////////
 //
 // WebDataRequestManager implementation.
 //
 ////////////////////////////////////////////////////////////////////////////////
 
 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();
+    i->second->MarkAsInactive();
   pending_requests_.clear();
 }
 
 void WebDataRequestManager::RegisterRequest(WebDataRequest* request) {
   base::AutoLock l(pending_lock_);
   pending_requests_[request->GetHandle()] = request;
 }
 
 int WebDataRequestManager::GetNextRequestHandle() {
   base::AutoLock l(pending_lock_);
   return ++next_request_handle_;

[Roger McFarlane (Chromium), 2016/12/13 22:33:55]

Maybe make this an atomic type?

As is, we take pending_lock_ twice on request creation: once to get the handle
and once to register the instance.

Could also make the handle non-const and set it after creating the request in a
factory func. Hmmm...

[Roger McFarlane (Chromium), 2016/12/14 19:05:53]

added factory func.

 }
 
 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());

[Roger McFarlane (Chromium), 2016/12/13 22:33:55]

This DCHECK isn't valid.

Given concurrent cancellation/completion, both synchronized using pending_lock_,
if the completion grabs the lock first, it will remove the handle from
pending_requests and DCHECK on debug builds, crash in release.

-  i->second->Cancel();
+  if (i == pending_requests_.end())
+    return;
+  i->second->MarkAsInactive();
   pending_requests_.erase(i);
 }
 
 void WebDataRequestManager::RequestCompleted(
-    std::unique_ptr<WebDataRequest> request) {
-  scoped_refptr<base::SingleThreadTaskRunner> task_runner =
-      request->GetTaskRunner();
-  task_runner->PostTask(
+    std::unique_ptr<WebDataRequest> request,
+    std::unique_ptr<WDTypedResult> result) {
+  // Transfer ownership of the result to the request.
+  {
+    base::AutoLock l(pending_lock_);
+    request->SetResult(std::move(result));

[Avi (use Gerrit), 2016/12/14 04:40:39]

If we no longer have the WebDatabaseBackend call SetResult on the request, but
are doing it here, should we make SetResult private and only callable by the
WebDataRequestManager?

Alternately, why do the SetResult here? Can't we just pass it along via the Bind
call to RequestCompletedOnThread, and clear the result field out of
WebDataRequest?

[Roger McFarlane (Chromium), 2016/12/14 14:02:49]

Ah, yes... should have made that private. But passing it along via the Bind is
an even better idea!

I'll do that, send another patchset, and re-ping.

+  }
+  request->GetTaskRunner()->PostTask(
       FROM_HERE, base::Bind(&WebDataRequestManager::RequestCompletedOnThread,
                             this, base::Passed(&request)));
 }
 
 void WebDataRequestManager::RequestCompletedOnThread(
     std::unique_ptr<WebDataRequest> request) {
+  // Early exit if the request is cancelled.
   if (request->IsCancelled())

[Roger McFarlane (Chromium), 2016/12/13 22:33:55]

An optimization, to attempt to avoid contending for the lock if the request has
already been cancelled/completed. The code should be equally correct without
this check.

[Mathieu, 2016/12/14 01:13:46]

This would be a worthwhile comment for a reader that may wonder why it's checked
again a few lines below

[Roger McFarlane (Chromium), 2016/12/14 14:02:49]

This is the double-checked locking pattern. I'll add a comment to that effect.

https://en.wikipedia.org/wiki/Double-checked_locking

     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
+  // when notifying the consumer that the request has completed.
+  WebDataServiceConsumer* consumer = nullptr;
+  std::unique_ptr<WDTypedResult> result;
+
+  // Manipulate the pending_requests_ collection while holding the lock.
   {
     base::AutoLock l(pending_lock_);
+
+    // Re-check if the request was cancelled. It might have been cancelled in
+    // another thread before the lock was acquired.
+    if (request->IsCancelled())
+      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"));
+
     auto i = pending_requests_.find(request->GetHandle());
     DCHECK(i != pending_requests_.end());

[Roger McFarlane (Chromium), 2016/12/13 22:33:55]

This DCHECK isn't valid.

Given concurrent cancellation/completion, both synchronized using pending_lock_,
if the cancellation grabs the lock first, it will remove the handle from
pending_requests and DCHECK on debug builds, crash in release.

[Mathieu, 2016/12/14 01:13:46]

It's still in your change, should we remove it?

[Roger McFarlane (Chromium), 2016/12/14 14:02:49]

It should be valid now, since we re-check while holding the lock for whether the
request is cancelled or not.

 
     // Take ownership of the request object and remove it from the map.
     pending_requests_.erase(i);
+
+    // Remember the consumer and result for notification below.
+    consumer = request->GetConsumer();
+    result = request->GetResult();
+
+    // The request is no longer active.
+    request->MarkAsInactive();
   }
 
-  // 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"));
+  // 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"));
 
-  // Notify the consumer if needed.
-  if (!request->IsCancelled()) {
-    WebDataServiceConsumer* consumer = request->GetConsumer();
-    request->OnComplete();
-    if (consumer) {
-      consumer->OnWebDataServiceRequestDone(request->GetHandle(),
-                                            request->GetResult());
-    }
+    consumer->OnWebDataServiceRequestDone(request->GetHandle(),
+                                          std::move(result));
   }
-
 }