Fix destruction of device management backend jobs

If the backend was destroyed during the status callback, this would
result in a double delete. Unregister the job before running the
callback in order to resolve this.

BUG=none
TEST=unit test

Review URL: http://codereview.chromium.org/6825006

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@81082 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 41 insertions, 11 deletions

diff --git a/chrome/browser/policy/device_management_backend_impl.cc b/chrome/browser/policy/device_management_backend_impl.cc
index 55d7319..bc95c12 100644
--- a/chrome/browser/policy/device_management_backend_impl.cc
+++ b/chrome/browser/policy/device_management_backend_impl.cc
@@ -8,10 +8,10 @@
 #include <vector>
 
 #include "base/stringprintf.h"
-#include "net/base/escape.h"
-#include "net/url_request/url_request_status.h"
 #include "chrome/browser/policy/device_management_service.h"
 #include "chrome/common/chrome_version_info.h"
+#include "net/base/escape.h"
+#include "net/url_request/url_request_status.h"
 
 namespace policy {
 
@@ -98,9 +98,7 @@ std::string URLQueryParameters::Encode() {
 class DeviceManagementJobBase
     : public DeviceManagementService::DeviceManagementJob {
  public:
-  virtual ~DeviceManagementJobBase() {
-    backend_impl_->JobDone(this);
-  }
+  virtual ~DeviceManagementJobBase() {}
 
   // DeviceManagementJob overrides:
   virtual void HandleResponse(const net::URLRequestStatus& status,
@@ -176,6 +174,8 @@ void DeviceManagementJobBase::HandleResponse(
     const std::string& data) {
   // Delete ourselves when this is done.
   scoped_ptr<DeviceManagementJob> scoped_killer(this);
+  backend_impl_->JobDone(this);
+  backend_impl_ = NULL;
 
   if (status.status() != net::URLRequestStatus::SUCCESS) {
     OnError(DeviceManagementBackend::kErrorRequestFailed);
@@ -367,15 +367,13 @@ DeviceManagementBackendImpl::DeviceManagementBackendImpl(
 }
 
 DeviceManagementBackendImpl::~DeviceManagementBackendImpl() {
-  // Swap to a helper, so we don't interfere with the unregistration on delete.
-  JobSet to_be_deleted;
-  to_be_deleted.swap(pending_jobs_);
-  for (JobSet::iterator job(to_be_deleted.begin());
-       job != to_be_deleted.end();
+  for (JobSet::iterator job(pending_jobs_.begin());
+       job != pending_jobs_.end();
        ++job) {
     service_->RemoveJob(*job);
     delete *job;
   }
+  pending_jobs_.clear();
 }
 
 std::string DeviceManagementBackendImpl::GetAgentString() {
diff --git a/chrome/browser/policy/device_management_service.cc b/chrome/browser/policy/device_management_service.cc
index 27fe57a..be8c151 100644
--- a/chrome/browser/policy/device_management_service.cc
+++ b/chrome/browser/policy/device_management_service.cc
@@ -188,8 +188,8 @@ void DeviceManagementService::OnURLFetchComplete(
   JobFetcherMap::iterator entry(pending_jobs_.find(source));
   if (entry != pending_jobs_.end()) {
     DeviceManagementJob* job = entry->second;
-    job->HandleResponse(status, response_code, cookies, data);
     pending_jobs_.erase(entry);
+    job->HandleResponse(status, response_code, cookies, data);
   } else {
     NOTREACHED() << "Callback from foreign URL fetcher";
   }
diff --git a/chrome/browser/policy/device_management_service_unittest.cc b/chrome/browser/policy/device_management_service_unittest.cc
index 556b6c9..b501d2c 100644
--- a/chrome/browser/policy/device_management_service_unittest.cc
+++ b/chrome/browser/policy/device_management_service_unittest.cc
@@ -20,6 +20,8 @@
 #include "testing/gtest/include/gtest/gtest.h"
 
 using testing::_;
+using testing::IgnoreResult;
+using testing::InvokeWithoutArgs;
 
 namespace policy {
 
@@ -243,6 +245,11 @@ class QueryParams {
 
 class DeviceManagementServiceTest
     : public DeviceManagementServiceTestBase<testing::Test> {
+ public:
+  void ResetBackend() {
+    backend_.reset();
+  }
+
  protected:
   void CheckURLAndQueryParams(const GURL& request_url,
                               const std::string& request_type,
@@ -506,4 +513,29 @@ TEST_F(DeviceManagementServiceTest, CancelRequestAfterShutdown) {
   backend_.reset();
 }
 
+TEST_F(DeviceManagementServiceTest, CancelDuringCallback) {
+  // Make a request.
+  DeviceRegisterResponseDelegateMock mock;
+  EXPECT_CALL(mock, OnError(_))
+      .WillOnce(InvokeWithoutArgs(this,
+                                  &DeviceManagementServiceTest::ResetBackend))
+      .RetiresOnSaturation();
+  em::DeviceRegisterRequest request;
+  backend_->ProcessRegisterRequest(kAuthToken, kDeviceId, request, &mock);
+  TestURLFetcher* fetcher = factory_.GetFetcherByID(0);
+  ASSERT_TRUE(fetcher);
+
+  // Generate a callback.
+  net::URLRequestStatus status(net::URLRequestStatus::SUCCESS, 0);
+  fetcher->delegate()->OnURLFetchComplete(fetcher,
+                                          GURL(kServiceUrl),
+                                          status,
+                                          500,
+                                          ResponseCookies(),
+                                          "");
+
+  // Backend should have been reset.
+  EXPECT_FALSE(backend_.get());
+}
+
 }  // namespace policy