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

This CL makes URLFetcherFileWriter::Finish() skip closing file when there is an
error. If there is an error, Finish() will delete the file after any pending
operation completes.

TBR=hashimoto@chromium.org
TBR=mkwst@chromium.org
BUG=487732, 656692

Review-Url: https://chromiumcodereview.appspot.com/2425673006
Cr-Commit-Position: refs/heads/master@{#426540}

chrome/browser/devtools/devtools_ui_bindings.cc

@@ -249,7 +249,7 @@ class ResponseWriter : public net::URLFetcherResponseWriter {
   int Write(net::IOBuffer* buffer,
             int num_bytes,
             const net::CompletionCallback& callback) override;
-  int Finish(const net::CompletionCallback& callback) override;
+  int Finish(int net_error, const net::CompletionCallback& callback) override;
 
  private:
   base::WeakPtr<DevToolsUIBindings> bindings_;
@@ -293,7 +293,8 @@ int ResponseWriter::Write(net::IOBuffer* buffer,
   return num_bytes;
 }
 
-int ResponseWriter::Finish(const net::CompletionCallback& callback) {
+int ResponseWriter::Finish(int net_error,
+                           const net::CompletionCallback& callback) {
   return net::OK;
 }
 

components/precache/core/precache_fetcher.cc

@@ -156,7 +156,7 @@ class URLFetcherNullWriter : public net::URLFetcherResponseWriter {
     return num_bytes;
   }
 
-  int Finish(const net::CompletionCallback& callback) override {
+  int Finish(int net_error, const net::CompletionCallback& callback) override {
     return net::OK;
   }
 };

content/shell/browser/shell_devtools_frontend.cc

@@ -49,7 +49,7 @@ class ResponseWriter : public net::URLFetcherResponseWriter {
   int Write(net::IOBuffer* buffer,
             int num_bytes,
             const net::CompletionCallback& callback) override;
-  int Finish(const net::CompletionCallback& callback) override;
+  int Finish(int net_error, const net::CompletionCallback& callback) override;
 
  private:
   base::WeakPtr<ShellDevToolsFrontend> shell_devtools_;
@@ -90,7 +90,8 @@ int ResponseWriter::Write(net::IOBuffer* buffer,
   return num_bytes;
 }
 
-int ResponseWriter::Finish(const net::CompletionCallback& callback) {
+int ResponseWriter::Finish(int net_error,
+                           const net::CompletionCallback& callback) {
   return net::OK;
 }
 

google_apis/drive/base_requests.cc

@@ -312,9 +312,10 @@ int ResponseWriter::Write(net::IOBuffer* buffer,
   return num_bytes;
 }
 
-int ResponseWriter::Finish(const net::CompletionCallback& callback) {
+int ResponseWriter::Finish(int net_error,
+                           const net::CompletionCallback& callback) {
   if (file_writer_)
-    return file_writer_->Finish(callback);
+    return file_writer_->Finish(net_error, callback);
 
   return net::OK;
 }

google_apis/drive/base_requests.h

@@ -135,7 +135,7 @@ class ResponseWriter : public net::URLFetcherResponseWriter {
   int Write(net::IOBuffer* buffer,
             int num_bytes,
             const net::CompletionCallback& callback) override;
-  int Finish(const net::CompletionCallback& callback) override;
+  int Finish(int net_error, const net::CompletionCallback& callback) override;
 
  private:
   void DidWrite(scoped_refptr<net::IOBuffer> buffer,

net/url_request/test_url_fetcher_factory.cc

@@ -188,7 +188,7 @@ void TestURLFetcher::SaveResponseWithWriter(
                                        fake_response_string_.size(),
                                        CompletionCallback());
     DCHECK_EQ(static_cast<int>(fake_response_string_.size()), response);
-    response = response_writer_->Finish(CompletionCallback());
+    response = response_writer_->Finish(OK, CompletionCallback());
     DCHECK_EQ(OK, response);
   } else if (fake_response_destination_ == TEMP_FILE) {
     // SaveResponseToFileAtPath() should be called instead of this method to

net/url_request/url_fetcher_core.cc

@@ -477,6 +477,7 @@ void URLFetcherCore::OnReadCompleted(URLRequest* request,
 
     // No more data to write.
     const int result = response_writer_->Finish(
+        bytes_read > 0 ? OK : bytes_read,
         base::Bind(&URLFetcherCore::DidFinishWriting, this));
     if (result != ERR_IO_PENDING)
       DidFinishWriting(result);
@@ -861,7 +862,7 @@ int URLFetcherCore::WriteBuffer(scoped_refptr<DrainableIOBuffer> data) {
 void URLFetcherCore::DidWriteBuffer(scoped_refptr<DrainableIOBuffer> data,
                                     int result) {
   if (result < 0) {  // Handle errors.
-    response_writer_->Finish(base::Bind(&EmptyCompletionCallback));
+    response_writer_->Finish(result, base::Bind(&EmptyCompletionCallback));
     CancelRequestAndInformDelegate(result);
     return;
   }

net/url_request/url_fetcher_response_writer.cc

@@ -4,6 +4,7 @@
 
 #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"
@@ -40,7 +41,8 @@ int URLFetcherStringWriter::Write(IOBuffer* buffer,
   return num_bytes;
 }
 
-int URLFetcherStringWriter::Finish(const CompletionCallback& callback) {
+int URLFetcherStringWriter::Finish(int net_error,
+                                   const CompletionCallback& callback) {
   // Do nothing.
   return OK;
 }
@@ -64,9 +66,12 @@ URLFetcherFileWriter::~URLFetcherFileWriter() {
 }
 
 int URLFetcherFileWriter::Initialize(const CompletionCallback& callback) {
+  DCHECK(!callback_);
+
   file_stream_.reset(new FileStream(file_task_runner_));
 
   int result = ERR_IO_PENDING;
+  owns_file_ = true;
   if (file_path_.empty()) {
     base::FilePath* temp_file_path = new base::FilePath;
     base::PostTaskAndReplyWithResult(
@@ -75,18 +80,22 @@ int URLFetcherFileWriter::Initialize(const CompletionCallback& callback) {
         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 |
+    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;
 }
 
@@ -95,24 +104,43 @@ int URLFetcherFileWriter::Write(IOBuffer* buffer,
                                 const CompletionCallback& callback) {
   DCHECK(file_stream_);
   DCHECK(owns_file_);
+  DCHECK(!callback_);
 
-  int result = file_stream_->Write(buffer, num_bytes,
-                                   base::Bind(&URLFetcherFileWriter::DidWrite,
-                                              weak_factory_.GetWeakPtr(),
-                                              callback));
-  if (result < 0 && result != ERR_IO_PENDING)
+  int result = file_stream_->Write(
+      buffer, num_bytes, base::Bind(&URLFetcherFileWriter::OnIOCompleted,
+                                    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 callback and invalid weak ptrs so as to cancel any posted task.
+    callback_.Reset();
+    weak_factory_.InvalidateWeakPtrs();
+    CloseAndDeleteFile();
+    return OK;
+  }
+  DCHECK(!callback_);
   // 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)
+        &URLFetcherFileWriter::CloseComplete, weak_factory_.GetWeakPtr()));
+    if (result == ERR_IO_PENDING) {
+      callback_ = callback;
+      return result;
+    }
     file_stream_.reset();
     return result;
   }
@@ -131,14 +159,6 @@ void URLFetcherFileWriter::DisownFile() {
   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;
@@ -151,41 +171,35 @@ void URLFetcherFileWriter::CloseAndDeleteFile() {
                                          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::File::FLAG_WRITE | base::File::FLAG_ASYNC | base::File::FLAG_OPEN,
+      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

net/url_request/url_fetcher_response_writer.h

@@ -45,9 +45,14 @@ class NET_EXPORT URLFetcherResponseWriter {
                     int num_bytes,
                     const CompletionCallback& callback) = 0;
 
-  // Finishes writing. If ERR_IO_PENDING is returned, |callback| will be run
-  // later with the result.
-  virtual int Finish(const CompletionCallback& callback) = 0;
+  // Finishes writing. If |net_error| is not OK, this method can be called
+  // in the middle of another operation (eg. Initialize() and Write()). On
+  // errors (|net_error| not OK), this method may be called before the previous
+  // operation completed. In this case, URLFetcherResponseWriter may skip
+  // graceful shutdown and completion of the pending operation. After such a
+  // failure, the URLFetcherResponseWriter may be reused. If ERR_IO_PENDING is
+  // returned, |callback| will be run later with the result.
+  virtual int Finish(int net_error, const CompletionCallback& callback) = 0;
 
   // Returns this instance's pointer as URLFetcherStringWriter when possible.
   virtual URLFetcherStringWriter* AsStringWriter();
@@ -69,7 +74,7 @@ class NET_EXPORT URLFetcherStringWriter : public URLFetcherResponseWriter {
   int Write(IOBuffer* buffer,
             int num_bytes,
             const CompletionCallback& callback) override;
-  int Finish(const CompletionCallback& callback) override;
+  int Finish(int net_error, const CompletionCallback& callback) override;
   URLFetcherStringWriter* AsStringWriter() override;
 
  private:
@@ -95,7 +100,7 @@ class NET_EXPORT URLFetcherFileWriter : public URLFetcherResponseWriter {
   int Write(IOBuffer* buffer,
             int num_bytes,
             const CompletionCallback& callback) override;
-  int Finish(const CompletionCallback& callback) override;
+  int Finish(int net_error, const CompletionCallback& callback) override;
   URLFetcherFileWriter* AsFileWriter() override;
 
   // Drops ownership of the file at |file_path_|.
@@ -103,23 +108,18 @@ class NET_EXPORT URLFetcherFileWriter : public URLFetcherResponseWriter {
   void DisownFile();
 
  private:
-  // Called when a write has been done.
-  void DidWrite(const CompletionCallback& callback, int result);
-
   // Closes the file if it is open and then delete it.
   void CloseAndDeleteFile();
 
   // Callback which gets the result of a temporary file creation.
-  void DidCreateTempFile(const CompletionCallback& callback,
-                         base::FilePath* temp_file_path,
-                         bool success);
+  void DidCreateTempFile(base::FilePath* temp_file_path, bool success);
 
-  // Callback which gets the result of FileStream::Open.
-  void DidOpenFile(const CompletionCallback& callback,
-                   int result);
+  // Run |callback_| if it is non-null when FileStream::Open or
+  // FileStream::Write is completed.
+  void OnIOCompleted(int result);
 
   // Callback which gets the result of closing a file.
-  void CloseComplete(const CompletionCallback& callback, int result);
+  void CloseComplete(int result);
 
   // Task runner on which file operations should happen.
   scoped_refptr<base::SequencedTaskRunner> file_task_runner_;
@@ -133,6 +133,8 @@ class NET_EXPORT URLFetcherFileWriter : public URLFetcherResponseWriter {
 
   std::unique_ptr<FileStream> file_stream_;
 
+  CompletionCallback callback_;
+
   // Callbacks are created for use with base::FileUtilProxy.
   base::WeakPtrFactory<URLFetcherFileWriter> weak_factory_;
 

net/url_request/url_fetcher_response_writer_unittest.cc

@@ -44,7 +44,7 @@ TEST_F(URLFetcherStringWriterTest, Basic) {
   EXPECT_THAT(callback.GetResult(rv), IsOk());
   rv = writer_->Write(buf_.get(), buf_->size(), callback.callback());
   EXPECT_EQ(buf_->size(), callback.GetResult(rv));
-  rv = writer_->Finish(callback.callback());
+  rv = writer_->Finish(OK, callback.callback());
   EXPECT_THAT(callback.GetResult(rv), IsOk());
 
   // Verify the result.
@@ -80,7 +80,7 @@ TEST_F(URLFetcherFileWriterTest, WriteToFile) {
   EXPECT_THAT(callback.GetResult(rv), IsOk());
   rv = writer_->Write(buf_.get(), buf_->size(), callback.callback());
   EXPECT_EQ(buf_->size(), callback.GetResult(rv));
-  rv = writer_->Finish(callback.callback());
+  rv = writer_->Finish(OK, callback.callback());
   EXPECT_THAT(callback.GetResult(rv), IsOk());
 
   // Verify the result.
@@ -103,7 +103,7 @@ TEST_F(URLFetcherFileWriterTest, InitializeAgain) {
   EXPECT_THAT(callback.GetResult(rv), IsOk());
   rv = writer_->Write(buf_.get(), buf_->size(), callback.callback());
   EXPECT_EQ(buf_->size(), callback.GetResult(rv));
-  rv = writer_->Finish(callback.callback());
+  rv = writer_->Finish(OK, callback.callback());
   EXPECT_THAT(callback.GetResult(rv), IsOk());
 
   // Verify the result.
@@ -119,7 +119,7 @@ TEST_F(URLFetcherFileWriterTest, InitializeAgain) {
   EXPECT_THAT(callback.GetResult(rv), IsOk());
   rv = writer_->Write(buf2.get(), buf2->size(), callback.callback());
   EXPECT_EQ(buf2->size(), callback.GetResult(rv));
-  rv = writer_->Finish(callback.callback());
+  rv = writer_->Finish(OK, callback.callback());
   EXPECT_THAT(callback.GetResult(rv), IsOk());
 
   // Verify the result.
@@ -128,13 +128,71 @@ TEST_F(URLFetcherFileWriterTest, InitializeAgain) {
   EXPECT_EQ(data2, file_contents);
 }
 
+TEST_F(URLFetcherFileWriterTest, FinishWhileWritePending) {
+  int rv = 0;
+  // Initialize(), Write() and Finish().
+  TestCompletionCallback callback;
+  rv = writer_->Initialize(callback.callback());
+  EXPECT_EQ(ERR_IO_PENDING, rv);
+  EXPECT_THAT(callback.WaitForResult(), IsOk());
+  TestCompletionCallback callback2;
+  rv = writer_->Write(buf_.get(), buf_->size(), callback2.callback());
+  EXPECT_EQ(ERR_IO_PENDING, rv);
+  TestCompletionCallback callback3;
+  rv = writer_->Finish(ERR_FAILED, callback3.callback());
+  EXPECT_EQ(OK, rv);
+
+  base::RunLoop().RunUntilIdle();
+  // Verify the result.
+  EXPECT_FALSE(base::PathExists(file_path_));
+}
+
+TEST_F(URLFetcherFileWriterTest, FinishWhileOpenPending) {
+  int rv = 0;
+  // Initialize() and Finish().
+  TestCompletionCallback callback;
+  rv = writer_->Initialize(callback.callback());
+  EXPECT_EQ(ERR_IO_PENDING, rv);
+  TestCompletionCallback callback2;
+  rv = writer_->Finish(ERR_FAILED, callback2.callback());
+  EXPECT_EQ(OK, rv);
+
+  base::RunLoop().RunUntilIdle();
+  // Verify the result.
+  EXPECT_FALSE(base::PathExists(file_path_));
+}
+
+TEST_F(URLFetcherFileWriterTest, InitializeAgainAfterFinishWithError) {
+  int rv = 0;
+  // Initialize(), Write() and Finish().
+  TestCompletionCallback callback;
+  rv = writer_->Initialize(callback.callback());
+  EXPECT_EQ(ERR_IO_PENDING, rv);
+  EXPECT_THAT(callback.WaitForResult(), IsOk());
+  TestCompletionCallback callback2;
+  rv = writer_->Write(buf_.get(), buf_->size(), callback2.callback());
+  EXPECT_EQ(ERR_IO_PENDING, rv);
+  TestCompletionCallback callback3;
+  rv = writer_->Finish(ERR_FAILED, callback3.callback());
+  EXPECT_EQ(OK, rv);
+
+  base::RunLoop().RunUntilIdle();
+  // Initialize() again and wait for it to complete.
+  TestCompletionCallback callback4;
+  rv = writer_->Initialize(callback4.callback());
+  EXPECT_EQ(ERR_IO_PENDING, rv);
+  EXPECT_THAT(callback4.WaitForResult(), IsOk());
+  // Verify the result.
+  EXPECT_TRUE(base::PathExists(file_path_));
+}
+
 TEST_F(URLFetcherFileWriterTest, DisownFile) {
   int rv = 0;
   // Initialize() and Finish() to create a file.
   TestCompletionCallback callback;
   rv = writer_->Initialize(callback.callback());
   EXPECT_THAT(callback.GetResult(rv), IsOk());
-  rv = writer_->Finish(callback.callback());
+  rv = writer_->Finish(OK, callback.callback());
   EXPECT_THAT(callback.GetResult(rv), IsOk());
 
   // Disown file.
@@ -166,7 +224,7 @@ TEST_F(URLFetcherFileWriterTemporaryFileTest, WriteToTemporaryFile) {
   EXPECT_THAT(callback.GetResult(rv), IsOk());
   rv = writer_->Write(buf_.get(), buf_->size(), callback.callback());
   EXPECT_EQ(buf_->size(), callback.GetResult(rv));
-  rv = writer_->Finish(callback.callback());
+  rv = writer_->Finish(OK, callback.callback());
   EXPECT_THAT(callback.GetResult(rv), IsOk());
 
   // Verify the result.

third_party/libaddressinput/chromium/chrome_metadata_source.cc

@@ -40,7 +40,8 @@ class UnownedStringWriter : public net::URLFetcherResponseWriter {
     return num_bytes;
   }
 
-  virtual int Finish(const net::CompletionCallback& callback) override {
+  virtual int Finish(int net_error,
+                     const net::CompletionCallback& callback) override {
     return net::OK;
   }