Make URLFetcherFileWriter::Finish() skip closing file when there is an error

net/url_request/url_fetcher_response_writer.cc

 // Copyright (c) 2013 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 "net/url_request/url_fetcher_response_writer.h"
 
+#include "base/callback_helpers.h"
 #include "base/files/file_util.h"
 #include "base/location.h"
 #include "base/sequenced_task_runner.h"
 #include "base/task_runner_util.h"
 #include "net/base/file_stream.h"
 #include "net/base/io_buffer.h"
 #include "net/base/net_errors.h"
 
 namespace net {
 
@@ skipping 16 matching lines @@
   return OK;
 }
 
 int URLFetcherStringWriter::Write(IOBuffer* buffer,
                                   int num_bytes,
                                   const CompletionCallback& callback) {
   data_.append(buffer->data(), num_bytes);
   return num_bytes;
 }
 
-int URLFetcherStringWriter::Finish(const CompletionCallback& callback) {
+int URLFetcherStringWriter::Finish(int net_error,
+                                   const CompletionCallback& callback) {
   // Do nothing.
   return OK;
 }
 
 URLFetcherStringWriter* URLFetcherStringWriter::AsStringWriter() {
   return this;
 }
 
 URLFetcherFileWriter::URLFetcherFileWriter(
     scoped_refptr<base::SequencedTaskRunner> file_task_runner,
     const base::FilePath& file_path)
     : file_task_runner_(file_task_runner),
       file_path_(file_path),
       owns_file_(false),
       weak_factory_(this) {
   DCHECK(file_task_runner_.get());
 }
 
 URLFetcherFileWriter::~URLFetcherFileWriter() {
   CloseAndDeleteFile();
 }
 
 int URLFetcherFileWriter::Initialize(const CompletionCallback& callback) {
   file_stream_.reset(new FileStream(file_task_runner_));

[mmenke, 2016/10/19 20:49:34]

DCHECK(!callback_);?  Same for all of these, except Finish()

[xunjieli, 2016/10/20 15:57:49]

Done.

 
   int result = ERR_IO_PENDING;
+  owns_file_ = true;
   if (file_path_.empty()) {
     base::FilePath* temp_file_path = new base::FilePath;
     base::PostTaskAndReplyWithResult(
         file_task_runner_.get(),
         FROM_HERE,
         base::Bind(&base::CreateTemporaryFile, temp_file_path),
         base::Bind(&URLFetcherFileWriter::DidCreateTempFile,
                    weak_factory_.GetWeakPtr(),
-                   callback,
                    base::Owned(temp_file_path)));
   } else {
     result = file_stream_->Open(
         file_path_,
         base::File::FLAG_WRITE | base::File::FLAG_ASYNC |
         base::File::FLAG_CREATE_ALWAYS,
-        base::Bind(&URLFetcherFileWriter::DidOpenFile,
-                   weak_factory_.GetWeakPtr(),
-                   callback));
+        base::Bind(&URLFetcherFileWriter::OnIOCompleted,
+                   weak_factory_.GetWeakPtr()));
     DCHECK_NE(OK, result);
   }
+
+  if (result == ERR_IO_PENDING) {
+    callback_ = callback;
+    return result;
+  }
+  if (result < 0)
+    CloseAndDeleteFile();
   return result;
 }
 
 int URLFetcherFileWriter::Write(IOBuffer* buffer,
                                 int num_bytes,
                                 const CompletionCallback& callback) {
   DCHECK(file_stream_);
   DCHECK(owns_file_);
 
   int result = file_stream_->Write(buffer, num_bytes,
-                                   base::Bind(&URLFetcherFileWriter::DidWrite,
-                                              weak_factory_.GetWeakPtr(),
-                                              callback));
-  if (result < 0 && result != ERR_IO_PENDING)
+                                   base::Bind(&URLFetcherFileWriter::OnIOCompleted,

[mmenke, 2016/10/19 20:49:34]

nit:  Line too long, run git cl format.

[xunjieli, 2016/10/20 15:57:49]

Done.

+                                              weak_factory_.GetWeakPtr()));
+  if (result == ERR_IO_PENDING) {
+    callback_ = callback;
+    return result;
+  }
+  if (result < 0)
     CloseAndDeleteFile();
-
   return result;
 }
 
-int URLFetcherFileWriter::Finish(const CompletionCallback& callback) {
+int URLFetcherFileWriter::Finish(int net_error,
+                                 const CompletionCallback& callback) {
+  DCHECK_NE(ERR_IO_PENDING, net_error);
+  // If an error occurred, simply delete the file after any pending operation
+  // is done. Do not call file_stream_->Close() because there might be an
+  // operation pending. See crbug.com/487732.
+  if (net_error < 0) {
+    // Cancel any pending callback.
+    callback_.Reset();
+    CloseAndDeleteFile();

[mmenke, 2016/10/19 20:49:34]

BUG:  If Finish() is called with an error when there's a pending operation, and
then Init is called before that operation completes, the completion of that
pending operation will cause the Callback() passed to Initialize to be invoked
before Initialize has been completed.

Not sure if we can reasonable write a test for this.

Regardless, should fix this issue.  I think the simplest fix is to just release
all weak ptrs the factory has?  I don't think that will get us into trouble. 
Does mean we don't need callback_ anymore, I think, though I'm fine with keeping
it (Or getting rid of it, no strong opinion).

[xunjieli, 2016/10/20 15:57:49]

Done. I haven't thought of that. That indeed can happen with the current setup.
Thanks for catching that!

+    return OK;
+  }

[mmenke, 2016/10/19 20:49:34]

DCHECK(!callback_);

[xunjieli, 2016/10/20 15:57:49]

Done.

   // If the file_stream_ still exists at this point, close it.
   if (file_stream_) {
     int result = file_stream_->Close(base::Bind(
-        &URLFetcherFileWriter::CloseComplete,
-        weak_factory_.GetWeakPtr(), callback));
-    if (result != ERR_IO_PENDING)
-      file_stream_.reset();
+        &URLFetcherFileWriter::CloseComplete, weak_factory_.GetWeakPtr()));
+    if (result == ERR_IO_PENDING) {
+      callback_ = callback;
+      return result;
+    }
+    file_stream_.reset();
     return result;
   }
   return OK;
 }
 
 URLFetcherFileWriter* URLFetcherFileWriter::AsFileWriter() {
   return this;
 }
 
 void URLFetcherFileWriter::DisownFile() {
   // Disowning is done by the delegate's OnURLFetchComplete method.
   // The file should be closed by the time that method is called.
   DCHECK(!file_stream_);
 
   owns_file_ = false;
 }
 
-void URLFetcherFileWriter::DidWrite(const CompletionCallback& callback,
-                                    int result) {
-  if (result < 0)
-    CloseAndDeleteFile();
-
-  callback.Run(result);
-}
-
 void URLFetcherFileWriter::CloseAndDeleteFile() {
   if (!owns_file_)
     return;
 
   file_stream_.reset();
   DisownFile();
   file_task_runner_->PostTask(FROM_HERE,
                               base::Bind(base::IgnoreResult(&base::DeleteFile),
                                          file_path_,
                                          false /* recursive */));
 }
 
-void URLFetcherFileWriter::DidCreateTempFile(const CompletionCallback& callback,
-                                             base::FilePath* temp_file_path,
+void URLFetcherFileWriter::DidCreateTempFile(base::FilePath* temp_file_path,
                                              bool success) {
   if (!success) {
-    callback.Run(ERR_FILE_NOT_FOUND);
+    OnIOCompleted(ERR_FILE_NOT_FOUND);
     return;
   }
   file_path_ = *temp_file_path;
-  owns_file_ = true;
   const int result = file_stream_->Open(
       file_path_,
       base::File::FLAG_WRITE | base::File::FLAG_ASYNC |
       base::File::FLAG_OPEN,
-      base::Bind(&URLFetcherFileWriter::DidOpenFile,
-                 weak_factory_.GetWeakPtr(),
-                 callback));
+      base::Bind(&URLFetcherFileWriter::OnIOCompleted,
+                 weak_factory_.GetWeakPtr()));
   if (result != ERR_IO_PENDING)
-    DidOpenFile(callback, result);
+    OnIOCompleted(result);
 }
 
-void URLFetcherFileWriter::DidOpenFile(const CompletionCallback& callback,
-                                       int result) {
-  if (result == OK)
-    owns_file_ = true;
-  else
+void URLFetcherFileWriter::OnIOCompleted(int result) {
+  if (result < OK)
     CloseAndDeleteFile();
 
-  callback.Run(result);
+  if (!callback_.is_null())
+     base::ResetAndReturn(&callback_).Run(result);
 }
 
-void URLFetcherFileWriter::CloseComplete(const CompletionCallback& callback,
-                                         int result) {
+void URLFetcherFileWriter::CloseComplete(int result) {
   // Destroy |file_stream_| whether or not the close succeeded.
   file_stream_.reset();
-  callback.Run(result);
+  if (!callback_.is_null())
+     base::ResetAndReturn(&callback_).Run(result);
 }
 
 }  // namespace net