Drive: Ensure call RequestFinished(this) in UrlFetchRequestBase.

UrlFetchRequestBase will not be deleted unless it calls
sender_->RequestFinished(this).  The CL adds missing call to fix memory
leak. Also the CL moves the flow of asynchrounous initialization from
MultipartUploadRequestBase to UrlFetchRequestBase so that UrlFetchRequestBase
can handle errors of the initialization.

BUG=471717
TEST=None

Review URL: https://codereview.chromium.org/1042893002

Cr-Commit-Position: refs/heads/master@{#322982}

2 files changed, 70 insertions, 28 deletions

diff --git a/google_apis/drive/base_requests.cc b/google_apis/drive/base_requests.cc
index 37a49e3..7f73abe 100644
--- a/google_apis/drive/base_requests.cc
+++ b/google_apis/drive/base_requests.cc
@@ -13,6 +13,7 @@
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/stringprintf.h"
 #include "base/task_runner_util.h"
+#include "base/thread_task_runner_handle.h"
 #include "base/values.h"
 #include "google_apis/drive/drive_api_parser.h"
 #include "google_apis/drive/request_sender.h"
@@ -270,17 +271,49 @@ void UrlFetchRequestBase::Start(const std::string& access_token,
   DCHECK(!access_token.empty());
   DCHECK(!callback.is_null());
   DCHECK(re_authenticate_callback_.is_null());
+  Prepare(base::Bind(&UrlFetchRequestBase::StartAfterPrepare,
+                     weak_ptr_factory_.GetWeakPtr(), access_token,
+                     custom_user_agent, callback));
+}
 
-  re_authenticate_callback_ = callback;
+void UrlFetchRequestBase::Prepare(const PrepareCallback& callback) {
+  DCHECK(CalledOnValidThread());
+  DCHECK(!callback.is_null());
+  callback.Run(HTTP_SUCCESS);
+}
 
-  GURL url = GetURL();
-  if (url.is_empty()) {
-    // Error is found on generating the url. Send the error message to the
-    // callback, and then return immediately without trying to connect
-    // to the server.
-    RunCallbackOnPrematureFailure(DRIVE_OTHER_ERROR);
+void UrlFetchRequestBase::StartAfterPrepare(
+    const std::string& access_token,
+    const std::string& custom_user_agent,
+    const ReAuthenticateCallback& callback,
+    DriveApiErrorCode code) {
+  DCHECK(CalledOnValidThread());
+  DCHECK(!access_token.empty());
+  DCHECK(!callback.is_null());
+  DCHECK(re_authenticate_callback_.is_null());
+
+  const GURL url = GetURL();
+  DriveApiErrorCode error_code;
+  if (code != HTTP_SUCCESS && code != HTTP_CREATED && code != HTTP_NO_CONTENT)
+    error_code = code;
+  else if (url.is_empty())
+    error_code = DRIVE_OTHER_ERROR;
+  else
+    error_code = HTTP_SUCCESS;
+
+  if (error_code != HTTP_SUCCESS) {
+    // Error is found on generating the url or preparing the request. Send the
+    // error message to the callback, and then return immediately without trying
+    // to connect to the server.  We need to call CompleteRequestWithError
+    // asynchronously because client code does not assume result callback is
+    // called synchronously.
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE, base::Bind(&UrlFetchRequestBase::CompleteRequestWithError,
+                              weak_ptr_factory_.GetWeakPtr(), error_code));
     return;
   }
+
+  re_authenticate_callback_ = callback;
   DVLOG(1) << "URL: " << url.spec();
 
   URLFetcher::RequestType request_type = GetRequestType();
@@ -380,8 +413,7 @@ void UrlFetchRequestBase::GetOutputFilePath(
 void UrlFetchRequestBase::Cancel() {
   response_writer_ = NULL;
   url_fetcher_.reset(NULL);
-  RunCallbackOnPrematureFailure(DRIVE_CANCELLED);
-  sender_->RequestFinished(this);
+  CompleteRequestWithError(DRIVE_CANCELLED);
 }
 
 DriveApiErrorCode UrlFetchRequestBase::GetErrorCode() {
@@ -473,11 +505,15 @@ void UrlFetchRequestBase::OnURLFetchComplete(const URLFetcher* source) {
   ProcessURLFetchResults(source);
 }
 
-void UrlFetchRequestBase::OnAuthFailed(DriveApiErrorCode code) {
+void UrlFetchRequestBase::CompleteRequestWithError(DriveApiErrorCode code) {
   RunCallbackOnPrematureFailure(code);
   sender_->RequestFinished(this);
 }
 
+void UrlFetchRequestBase::OnAuthFailed(DriveApiErrorCode code) {
+  CompleteRequestWithError(code);
+}
+
 base::WeakPtr<AuthenticatedRequestInterface>
 UrlFetchRequestBase::GetWeakPtr() {
   return weak_ptr_factory_.GetWeakPtr();
@@ -775,9 +811,7 @@ MultipartUploadRequestBase::MultipartUploadRequestBase(
 MultipartUploadRequestBase::~MultipartUploadRequestBase() {
 }
 
-void MultipartUploadRequestBase::Start(const std::string& access_token,
-                                       const std::string& custom_user_agent,
-                                       const ReAuthenticateCallback& callback) {
+void MultipartUploadRequestBase::Prepare(const PrepareCallback& callback) {
   // If the request is cancelled, the request instance will be deleted in
   // |UrlFetchRequestBase::Cancel| and OnPrepareUploadContent won't be called.
   std::string* const upload_content_type = new std::string();
@@ -788,25 +822,23 @@ void MultipartUploadRequestBase::Start(const std::string& access_token,
                  local_path_, base::Unretained(upload_content_type),
                  base::Unretained(upload_content_data)),
       base::Bind(&MultipartUploadRequestBase::OnPrepareUploadContent,
-                 weak_ptr_factory_.GetWeakPtr(), access_token,
-                 custom_user_agent, callback, base::Owned(upload_content_type),
+                 weak_ptr_factory_.GetWeakPtr(), callback,
+                 base::Owned(upload_content_type),
                  base::Owned(upload_content_data)));
 }
 
 void MultipartUploadRequestBase::OnPrepareUploadContent(
-    const std::string& access_token,
-    const std::string& custom_user_agent,
-    const ReAuthenticateCallback& callback,
+    const PrepareCallback& callback,
     std::string* upload_content_type,
     std::string* upload_content_data,
     bool result) {
   if (!result) {
-    RunCallbackOnPrematureFailure(DRIVE_FILE_ERROR);
+    callback.Run(DRIVE_FILE_ERROR);
     return;
   }
   upload_content_type_.swap(*upload_content_type);
   upload_content_data_.swap(*upload_content_data);
-  UrlFetchRequestBase::Start(access_token, custom_user_agent, callback);
+  callback.Run(HTTP_SUCCESS);
 }
 
 void MultipartUploadRequestBase::SetBoundaryForTesting(
diff --git a/google_apis/drive/base_requests.h b/google_apis/drive/base_requests.h
index d7f46ab..2cfddf0 100644
--- a/google_apis/drive/base_requests.h
+++ b/google_apis/drive/base_requests.h
@@ -30,6 +30,8 @@ namespace google_apis {
 class FileResource;
 class RequestSender;
 
+typedef base::Callback<void(DriveApiErrorCode)> PrepareCallback;
+
 // Callback used for requests that the server returns FileResource data
 // formatted into JSON value.
 typedef base::Callback<void(DriveApiErrorCode error,
@@ -140,6 +142,10 @@ class UrlFetchRequestBase : public AuthenticatedRequestInterface,
   explicit UrlFetchRequestBase(RequestSender* sender);
   ~UrlFetchRequestBase() override;
 
+  // Does async initialization for the request. |Start| calls this method so you
+  // don't need to call this before |Start|.
+  virtual void Prepare(const PrepareCallback& callback);
+
   // Gets URL for the request.
   virtual GURL GetURL() const = 0;
 
@@ -200,6 +206,16 @@ class UrlFetchRequestBase : public AuthenticatedRequestInterface,
   base::SequencedTaskRunner* blocking_task_runner() const;
 
  private:
+  // Continues |Start| function after |Prepare|.
+  void StartAfterPrepare(const std::string& access_token,
+                         const std::string& custom_user_agent,
+                         const ReAuthenticateCallback& callback,
+                         DriveApiErrorCode code);
+
+  // Invokes callback with |code| and request to delete the request to
+  // |sender_|.
+  void CompleteRequestWithError(DriveApiErrorCode code);
+
   // URLFetcherDelegate overrides.
   void OnURLFetchComplete(const net::URLFetcher* source) override;
 
@@ -458,12 +474,8 @@ class MultipartUploadRequestBase : public UrlFetchRequestBase {
                              const ProgressCallback& progress_callback);
   ~MultipartUploadRequestBase() override;
 
-  // Overridden from AuthenticatedRequestInterface.
-  void Start(const std::string& access_token,
-             const std::string& custom_user_agent,
-             const ReAuthenticateCallback& callback) override;
-
   // Overridden from UrlFetchRequestBase.
+  void Prepare(const PrepareCallback& callback) override;
   bool GetContentData(std::string* upload_content_type,
                       std::string* upload_content) override;
   void ProcessURLFetchResults(const net::URLFetcher* source) override;
@@ -480,9 +492,7 @@ class MultipartUploadRequestBase : public UrlFetchRequestBase {
  private:
   // Continues to rest part of |Start| method after determining boundary string
   // of multipart/related.
-  void OnPrepareUploadContent(const std::string& access_token,
-                              const std::string& custom_user_agent,
-                              const ReAuthenticateCallback& callback,
+  void OnPrepareUploadContent(const PrepareCallback& callback,
                               std::string* upload_content_type,
                               std::string* upload_content_data,
                               bool result);