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, FinishWhileWritePending) {
+  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(ERR_IO_PENDING, rv);
+  rv = writer_->Finish(ERR_FAILED, callback.callback());
+  EXPECT_EQ(OK, rv);
+
+  // Verify the result.
+  EXPECT_THAT(callback.GetResult(rv), IsOk());
+  base::RunLoop().RunUntilIdle();
+  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);
+  rv = writer_->Finish(ERR_FAILED, callback.callback());
+  EXPECT_EQ(OK, rv);
+
+  // Verify the result.
+  EXPECT_THAT(callback.GetResult(rv), IsOk());
+  base::RunLoop().RunUntilIdle();
+  EXPECT_FALSE(base::PathExists(file_path_));

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

Here's the line that fails.

CloseAndDelete() is an no-op if |owns_file_| is false. 
|owns_file_| is true only when Initialize completes asynchronously :(

Maybe we should change read access of |owns_file_| on the task runner thread?

[mmenke, 2016/10/18 21:40:46]

Should we just set owns file before each post task to
URLFetcherFileWriter::DidOpenFile?  If it fails with an error, we'll try to
delete the file, anyways, and aborting seems like kind of the same thing.

[xunjieli, 2016/10/19 12:00:21]

Done. Right I think that's equivalent and is better!

+}
+
+TEST_F(URLFetcherFileWriterTest, InitializeAgainAfterFinishWithError) {
+  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(ERR_IO_PENDING, rv);
+  rv = writer_->Finish(ERR_FAILED, callback.callback());
+  EXPECT_EQ(OK, rv);
+  // Wait for Write() to complete.
+  EXPECT_THAT(callback.GetResult(rv), IsOk());

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

Will using the same TestCompletionCallback be a problem? I see other tests doing
so. I don't suppose TestCompletionCallback resets its callback?

[mmenke, 2016/10/18 21:40:46]

We should use a different callback here - This should work, but seems a bad
idea.  If it were to be called as a result of the Write() call, we could
incorrectly pass the test (Admittedly, if Initialize() called it twice or
something, would have the same issue, but we generally don't worry about that). 
Using a different callback also lets us check that the one passed to Write is
never invoked as a result of the Write call.

[xunjieli, 2016/10/19 12:00:21]

Done.

+
+  // Verify the result.
+  rv = writer_->Initialize(callback.callback());
+  EXPECT_THAT(callback.GetResult(rv), IsOk());
+  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.
   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