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..1015a7ed699337a066d61d0d8fd9104474c76cb6 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,29 @@ TEST_F(URLFetcherFileWriterTest, InitializeAgain) {
   EXPECT_EQ(data2, file_contents);
 }
 
+TEST_F(URLFetcherFileWriterTest, FinishWhileOperationPending) {
+  int rv = 0;
+  // Initialize(), Write() and Finish().
+  TestCompletionCallback callback;
+  rv = writer_->Initialize(callback.callback());

[mmenke, 2016/10/18 20:20:02]

Maybe a test where Initialize is pending, too?  Don't want to test everything,
but initializing is magic.

[xunjieli, 2016/10/18 21:18:07]

Done. Ah I should have thought about that.

The new test actually caught a bug where CloseAndDelete() checks |owns_file_|
synchronously before Open() completes, so we won't delete the file in this case
(see comment below)

+  EXPECT_THAT(callback.GetResult(rv), IsOk());
+  rv = writer_->Write(buf_.get(), buf_->size(), callback.callback());
+  EXPECT_EQ(ERR_IO_PENDING, rv);
+  rv = writer_->Finish(ERR_FAILED, callback.callback());
+  EXPECT_EQ(OK, rv);
+
+  // Verify the result.
+  base::RunLoop().RunUntilIdle();
+  EXPECT_FALSE(base::PathExists(file_path_));

[mmenke, 2016/10/18 20:20:01]

Is it worth having a test where we Initialize() the writer again before spinning
the message loop, just out of paranoia?

[xunjieli, 2016/10/18 21:18:07]

Done.

+}
+
 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 +182,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.