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..0a4ee6563068ea20f085e8a79cef2e1dc52b131a 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,64 @@ 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());
+  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.
@@ -166,7 +217,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.