URLFetcher won't call delegate until file is closed.

net/url_request/url_fetcher_response_writer.cc

 // Copyright (c) 2013 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/file_util.h"
 #include "base/location.h"
 #include "base/task_runner.h"
 #include "base/task_runner_util.h"
@@ skipping 84 matching lines @@
                                               weak_factory_.GetWeakPtr(),
                                               callback));
   if (result < 0 && result != ERR_IO_PENDING) {
     error_code_ = result;
     CloseAndDeleteFile();
   }
   return result;
 }
 
 int URLFetcherFileWriter::Finish(const CompletionCallback& callback) {
+  int result = file_stream_->Close(base::Bind(
+      &URLFetcherFileWriter::CloseComplete, base::Unretained(this), callback));

[mmenke, 2013/09/10 18:32:52]

The base::Unretained changes the safe usage model of the URLFetcherFileWriter. 
Previously, it could safely be deleted at any point.  You should use
weak_factory_.GetWeakPtr() here.

If you look at URLFetcherCore::CancelURLRequest and the calls into it from
URLFetchImpl, this seems to be a property that is relevant.  I don't see any
tests for this property, but may be worth adding.

[waffles, 2013/09/10 21:41:34]

Done.

+  if (result != ERR_IO_PENDING)
+    file_stream_.reset();
+  return result;
+}
+
+void URLFetcherFileWriter::CloseComplete(const CompletionCallback& callback,
+                                         int result) {
+  // Either we closed the file or we can't; either way destroy the file_stream.
   file_stream_.reset();
-  return OK;
+  callback.Run(result);
 }
 
 void URLFetcherFileWriter::DidWrite(const CompletionCallback& callback,
                                     int result) {
   if (result < 0) {
     error_code_ = result;
     CloseAndDeleteFile();
   }
   callback.Run(result);
 }
 
 void URLFetcherFileWriter::DisownFile() {
   // Disowning is done by the delegate's OnURLFetchComplete method.
   // The file should be closed by the time that method is called.
   DCHECK(!file_stream_);
 
   owns_file_ = false;
 }
 
 void URLFetcherFileWriter::CloseAndDeleteFile() {
   if (!owns_file_)
     return;
 
   file_stream_.reset();
   DisownFile();
   file_task_runner_->PostTask(FROM_HERE,
                               base::Bind(base::IgnoreResult(&base::DeleteFile),

[mmenke, 2013/09/10 18:32:52]

Kinda related to this CL:  Think it's worth a comment that this is safe because
file_stream_.reset() will close the file on file_task_runner_ before this task
runs (Was trying to figure out if this was safe).

[waffles, 2013/09/10 21:41:34]

Hm, except that file_task_runner_ is only guaranteed to be a base::TaskRunner,
not a base::SequencedTaskRunner, and therefore may not perform tasks in FIFO
order, right?

So I think it's unsafe (and always has been).

[mmenke, 2013/09/10 22:25:16]

Looks like you're right.  It actually used to use message loops.  And the CL
that switched it over to using TaskRunners a year ago got it right, so it's
regressed sometime since then.

I'm thinking I'll do some more spelunking to figure out why and upload a
separate patch, unless you have an urge to fix it yourself, either in this CL or
in another (And I'll want a unit test).

[waffles, 2013/09/10 22:33:22]

I'd rather not fix it in this CL; I definitely don't mind doing it in another
but I'm not sure exactly when I'll have the cycles.

[mmenke, 2013/09/10 22:38:19]

That's fine, I'll do it.  I want to investigate why it regressed, to make sure
that there wasn't any bug fix involved, anyways.

                                          file_path_,
                                          false /* recursive */));
 }
 
 void URLFetcherFileWriter::DidCreateTempFile(const CompletionCallback& callback,
                                              base::FilePath* temp_file_path,
                                              bool success) {
   if (!success) {
     error_code_ = ERR_FILE_NOT_FOUND;
     callback.Run(error_code_);
@@ skipping 18 matching lines @@
     total_bytes_written_ = 0;
     owns_file_ = true;
   } else {
     error_code_ = result;
     CloseAndDeleteFile();
   }
   callback.Run(result);
 }
 
 }  // namespace net