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

net/url_request/url_fetcher_response_writer_unittest.cc

Index: net/url_request/url_fetcher_response_writer_unittest.cc
diff --git a/net/url_request/url_fetcher_response_writer_unittest.cc b/net/url_request/url_fetcher_response_writer_unittest.cc
index 54763187cc0a16e24fec480ef3078a668e5eeb08..8bd362fc200dd3e2bec08a5cea629654658f1cf9 100644
--- a/net/url_request/url_fetcher_response_writer_unittest.cc
+++ b/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,72 @@ 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_THAT(callback.GetResult(rv), 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);
+
+  // Write() should finish asynchronously.
+  EXPECT_THAT(callback2.GetResult(rv), IsOk());

[mmenke, 2016/10/19 14:18:40]

I don't think we want the callback to be invoked here (i.e., Finish() with an
error should abort any other pending callback)

[xunjieli, 2016/10/19 15:19:28]

Done. Changed implementation to cancel pending callback.

+  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);
+
+  // Initialize() should finish asynchronously.
+  EXPECT_THAT(callback.GetResult(rv), IsOk());
+  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_THAT(callback.GetResult(rv), 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);
+  // Wait for Write() to complete.
+  EXPECT_THAT(callback.GetResult(rv), IsOk());
+
+  // Initialize() again and wait for it to complete.
+  rv = writer_->Initialize(callback.callback());
+  EXPECT_THAT(callback.GetResult(rv), 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 +225,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.