Fix WebDataRequest ownership gap

chrome/browser/webdata/web_data_request_manager.cc

Index: chrome/browser/webdata/web_data_request_manager.cc
diff --git a/chrome/browser/webdata/web_data_request_manager.cc b/chrome/browser/webdata/web_data_request_manager.cc
index cf3e6130ff396aef213012cf3cbef73e8d00c656..f6326f494cd49ae58a43a8ee52cea82afeb7a8eb 100644
--- a/chrome/browser/webdata/web_data_request_manager.cc
+++ b/chrome/browser/webdata/web_data_request_manager.cc
@@ -7,8 +7,6 @@
 #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"
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -21,15 +19,17 @@ 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 {
@@ -39,6 +39,9 @@ WebDataService::Handle WebDataRequest::GetHandle() const {
 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_);
@@ -59,11 +62,6 @@ const WDTypedResult* WebDataRequest::GetResult() const {
   return result_.get();
 }
 
-void WebDataRequest::RequestComplete() {
-  message_loop_->PostTask(FROM_HERE, Bind(&WebDataService::RequestCompleted,
-                                          service_.get(), handle_));
-}
-
 ////////////////////////////////////////////////////////////////////////////////
 //
 // WebDataRequestManager implementation.
@@ -75,6 +73,12 @@ WebDataRequestManager::WebDataRequestManager()
 }
 
 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) {
@@ -87,19 +91,35 @@ int WebDataRequestManager::GetNextRequestHandle() {
   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();
@@ -107,7 +127,6 @@ void WebDataRequestManager::RequestCompleted(WebDataServiceBase::Handle h) {
   }
 
   // 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();