Fix WebDataRequest ownership gap

chrome/browser/webdata/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 "chrome/browser/webdata/web_data_request_manager.h"
 
 #include "base/bind.h"
 #include "base/message_loop.h"
 #include "base/stl_util.h"
-#include "chrome/browser/autofill/autofill_profile.h"
-#include "chrome/browser/autofill/credit_card.h"
 #include "chrome/browser/webdata/web_data_service.h"
 
 ////////////////////////////////////////////////////////////////////////////////
 //
 // WebDataRequest implementation.
 //
 ////////////////////////////////////////////////////////////////////////////////
 
 WebDataRequest::WebDataRequest(WebDataService* service,
                                WebDataServiceConsumer* consumer,
                                WebDataRequestManager* manager)
     : service_(service),
+      manager_(manager),
       cancelled_(false),
       consumer_(consumer),
       result_(NULL) {
-  handle_ = manager->GetNextRequestHandle();
+  handle_ = manager_->GetNextRequestHandle();
   message_loop_ = MessageLoop::current();
-  manager->RegisterRequest(this);
+  manager_->RegisterRequest(this);
 }
 
 WebDataRequest::~WebDataRequest() {
+  manager_->MaybeCancelRequest(handle_);
 }
 
 WebDataService::Handle WebDataRequest::GetHandle() const {
   return handle_;
 }
 
 WebDataServiceConsumer* WebDataRequest::GetConsumer() const {
   return consumer_;
 }

[dhollowa, 2013/01/16 18:41:29]

nit: extra line is needed here.

[Cait (Slow), 2013/01/24 22:45:51]

Done.

+MessageLoop* WebDataRequest::GetMessageLoop() const {
+  return message_loop_;
+}
 
 bool WebDataRequest::IsCancelled() const {
   base::AutoLock l(cancel_lock_);
   return cancelled_;
 }
 
 void WebDataRequest::Cancel() {
   base::AutoLock l(cancel_lock_);
   cancelled_ = true;
   consumer_ = NULL;
 }
 
 void WebDataRequest::SetResult(scoped_ptr<WDTypedResult> r) {
   result_ = r.Pass();
 }
 
 const WDTypedResult* WebDataRequest::GetResult() const {
   return result_.get();
 }
 
-void WebDataRequest::RequestComplete() {
-  message_loop_->PostTask(FROM_HERE, Bind(&WebDataService::RequestCompleted,
-                                          service_.get(), handle_));
-}
-
 ////////////////////////////////////////////////////////////////////////////////
 //
 // WebDataRequestManager implementation.
 //
 ////////////////////////////////////////////////////////////////////////////////
 
 WebDataRequestManager::WebDataRequestManager()
     : next_request_handle_(1) {
 }
 
 WebDataRequestManager::~WebDataRequestManager() {
+  base::AutoLock l(pending_lock_);
+  for (RequestMap::iterator i = pending_requests_.begin();
+       i != pending_requests_.end(); ++i) {
+    i->second->Cancel();

[erikwright (departed), 2013/01/16 19:00:23]

I think it's a requirement that WebDataRequest::Cancel() NULL-out the pointer to
the manager (and that ~WebDataRequest behave correctly at that time).

Otherwise you have a use-after-free.

[Cait (Slow), 2013/01/24 22:45:51]

Done.

+  }
+  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_;
 }
 
-void WebDataRequestManager::CancelRequest(WebDataServiceBase::Handle h) {
+bool WebDataRequestManager::MaybeCancelRequest(WebDataServiceBase::Handle h) {
   base::AutoLock l(pending_lock_);
   RequestMap::iterator i = pending_requests_.find(h);
   if (i == pending_requests_.end()) {

[dhollowa, 2013/01/16 18:41:29]

nit: no longer need {} for single line body.

[Cait (Slow), 2013/01/24 22:45:51]

Done.

-    NOTREACHED() << "Canceling a nonexistent web data service request";
-    return;
+    return false;
   }
   i->second->Cancel();

[erikwright (departed), 2013/01/16 19:00:23]

It seems like a bug that 'i' is not erased from pending_requests_ here.

[Cait (Slow), 2013/01/24 22:45:51]

Done -- This causes a bit of weirdness in the case that the request is cancelled
in between calls to RequestCompleted and ReqCompletedOnThread. We end up in
ReqCompletedOnThread witha request no longer in the pending_requests_. For now
I'm adding an isCancelled check with an early exit.

+  return true;
 }
 
-void WebDataRequestManager::RequestCompleted(WebDataServiceBase::Handle h) {
+void WebDataRequestManager::CancelRequest(WebDataServiceBase::Handle h) {
+  if (!MaybeCancelRequest(h)) {
+    NOTREACHED() << "Canceling a nonexistent web data service request";

[dhollowa, 2013/01/16 18:41:29]

nit: no longer need {} for single line body.

[Cait (Slow), 2013/01/24 22:45:51]

Done.

+  }
+}
+
+void WebDataRequestManager::RequestCompleted(
+    scoped_ptr<WebDataRequest> request) {
+  MessageLoop* loop = request->GetMessageLoop();
+  loop->PostTask(FROM_HERE, base::Bind(
+      &WebDataRequestManager::RequestCompletedOnThread,
+      base::Unretained(this),
+      base::Passed(&request)));
+}
+
+void WebDataRequestManager::RequestCompletedOnThread(
+    scoped_ptr<WebDataRequest> request) {
   pending_lock_.Acquire();
-  RequestMap::iterator i = pending_requests_.find(h);
+  RequestMap::iterator i = pending_requests_.find(request->GetHandle());
   if (i == pending_requests_.end()) {
     NOTREACHED() << "Request completed called for an unknown request";
     pending_lock_.Release();
     return;
   }
 
   // Take ownership of the request object and remove it from the map.
-  scoped_ptr<WebDataRequest> request(i->second);
   pending_requests_.erase(i);
   pending_lock_.Release();
 
   // Notify the consumer if needed.
   WebDataServiceConsumer* consumer = request->GetConsumer();
   if (!request->IsCancelled() && consumer) {
     consumer->OnWebDataServiceRequestDone(request->GetHandle(),
                                           request->GetResult());

[erikwright (departed), 2013/01/16 19:00:23]

Make GetResult transfer ownership of the WDTypedResult to the caller (i.e.,
return a scoped_ptr<WDTypedResult>).

Continue to pass the raw pointer to consumers, but that will lead to the result
being freed at the end of this scope. And move the call to
WDTypedResult::Destroy into ~WDTypedResult.

Then the 'else' block can be removed.

[Cait (Slow), 2013/01/24 22:45:51]

Done.

   } else {
     // Nobody is taken ownership of the result, either because it is cancelled
     // or there is no consumer. Destroy results that require special handling.
     WDTypedResult const *result = request->GetResult();
     if (result) {
       result->Destroy();

[erikwright (departed), 2013/01/16 19:00:23]

As an aside, rather than passing an optional callback into WDTResult<T>(), why
not subclass it where it is necessary and implement the callback behaviour in
the subclass's destructor, especially now that the callback would only ever be
invoked from there.

But it would be even better to eliminate the WDTypedResult and WebDataConsumer
classes, replace the consumer with callbacks, and be done with it.

Obviously, one way or the other for a future CL.

[Cait (Slow), 2013/01/24 22:45:51]

Done.

     }
   }

[erikwright (departed), 2013/01/16 19:00:23]

I think something at this point must tell the request that it is complete,
should release its pointer to the manager, and should not call
MaybeCancelRequest.

For example request->OnComplete() or something.

And therefore, MaybeCancelRequest can be factored out, and ~WebDataRequest can
just call CancelRequest.

[Cait (Slow), 2013/01/24 22:45:51]

Done.

 }