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

net/url_request/url_fetcher_response_writer_unittest.cc

 // Copyright 2014 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/files/file_util.h"
 #include "base/files/scoped_temp_dir.h"
 #include "base/run_loop.h"
 #include "base/threading/thread_task_runner_handle.h"
@@ skipping 26 matching lines @@
 };
 
 TEST_F(URLFetcherStringWriterTest, Basic) {
   int rv = 0;
   // Initialize(), Write() and Finish().
   TestCompletionCallback callback;
   rv = writer_->Initialize(callback.callback());
   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.
   EXPECT_EQ(kData, writer_->data());
 
   // Initialize() again to reset.
   rv = writer_->Initialize(callback.callback());
   EXPECT_THAT(callback.GetResult(rv), IsOk());
   EXPECT_TRUE(writer_->data().empty());
 }
@@ skipping 15 matching lines @@
 };
 
 TEST_F(URLFetcherFileWriterTest, WriteToFile) {
   int rv = 0;
   // Initialize(), Write() and Finish().
   TestCompletionCallback callback;
   rv = writer_->Initialize(callback.callback());
   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.
   EXPECT_EQ(file_path_.value(), writer_->file_path().value());
   std::string file_contents;
   EXPECT_TRUE(base::ReadFileToString(writer_->file_path(), &file_contents));
   EXPECT_EQ(kData, file_contents);
 
   // Destroy the writer. File should be deleted.
   writer_.reset();
   base::RunLoop().RunUntilIdle();
   EXPECT_FALSE(base::PathExists(file_path_));
 }
 
 TEST_F(URLFetcherFileWriterTest, InitializeAgain) {
   int rv = 0;
   // Initialize(), Write() and Finish().
   TestCompletionCallback callback;
   rv = writer_->Initialize(callback.callback());
   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.
   std::string file_contents;
   EXPECT_TRUE(base::ReadFileToString(writer_->file_path(), &file_contents));
   EXPECT_EQ(kData, file_contents);
 
   // Initialize() again to reset. Write different data.
   const std::string data2 = "Bye!";
   scoped_refptr<StringIOBuffer> buf2(new StringIOBuffer(data2));
 
   rv = writer_->Initialize(callback.callback());
   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.
   file_contents.clear();
   EXPECT_TRUE(base::ReadFileToString(writer_->file_path(), &file_contents));
   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.
   writer_->DisownFile();
 
   // File is not deleted even after the writer gets destroyed.
   writer_.reset();
   base::RunLoop().RunUntilIdle();
   EXPECT_TRUE(base::PathExists(file_path_));
 }
@@ skipping 11 matching lines @@
 };
 
 TEST_F(URLFetcherFileWriterTemporaryFileTest, WriteToTemporaryFile) {
   int rv = 0;
   // Initialize(), Write() and Finish().
   TestCompletionCallback callback;
   rv = writer_->Initialize(callback.callback());
   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.
   std::string file_contents;
   EXPECT_TRUE(base::ReadFileToString(writer_->file_path(), &file_contents));
   EXPECT_EQ(kData, file_contents);
 
   // Destroy the writer. File should be deleted.
   const base::FilePath file_path = writer_->file_path();
   writer_.reset();
   base::RunLoop().RunUntilIdle();
   EXPECT_FALSE(base::PathExists(file_path));
 }
 
 }  // namespace net