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

net/url_request/url_fetcher_core.cc

 // Copyright (c) 2012 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_core.h"
 
 #include <stdint.h>
 #include <utility>
 
 #include "base/bind.h"
@@ skipping 459 matching lines @@
   // See comments re: HEAD requests in ReadResponse().
   if (bytes_read != ERR_IO_PENDING || request_type_ == URLFetcher::HEAD) {
     status_ = URLRequestStatus::FromError(bytes_read);
     received_response_content_length_ =
         request_->received_response_content_length();
     total_received_bytes_ += request_->GetTotalReceivedBytes();
     ReleaseRequest();
 
     // No more data to write.
     const int result = response_writer_->Finish(
+        bytes_read > 0 ? OK : bytes_read,

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

Hrm...I'd normally ask for an integration test that runs into the problem, but
I'd actually really like to fix URLRequest so that this can't happen.  Fixing
the API would break the test.  (And I'd like to fix the API sooner rather than
later)

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

I see. So in the future URLRequest::Delegate::OnReadCompleted() won't be called
with an net error (bytes_read < 0)?

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

No, in the future, I want it not to call into the Delegate with an error unless
there's a pending read/start operation, which I think this the only case this
happens in?

Cancellation through the URLFetcher interface goes through a different path, and
doesn't run into this problem, I believe.  And this code doesn't currently
open/write to the file while trying to read remote data.

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

Acknowledged. Thanks for explaining!

         base::Bind(&URLFetcherCore::DidFinishWriting, this));
     if (result != ERR_IO_PENDING)
       DidFinishWriting(result);
   }
 }
 
 void URLFetcherCore::OnContextShuttingDown() {
   DCHECK(request_);
   CancelRequestAndInformDelegate(ERR_CONTEXT_SHUT_DOWN);
 }
@@ skipping 364 matching lines @@
       return result;
     }
     data->DidConsume(result);
   }
   return OK;
 }
 
 void URLFetcherCore::DidWriteBuffer(scoped_refptr<DrainableIOBuffer> data,
                                     int result) {
   if (result < 0) {  // Handle errors.
-    response_writer_->Finish(base::Bind(&EmptyCompletionCallback));
+    response_writer_->Finish(result, base::Bind(&EmptyCompletionCallback));
     CancelRequestAndInformDelegate(result);
     return;
   }
 
   // Continue writing.
   data->DidConsume(result);
   if (WriteBuffer(data) < 0)
     return;
 
   // Finished writing buffer_. Read some more, unless the request has been
@@ skipping 73 matching lines @@
 }
 
 void URLFetcherCore::AssertHasNoUploadData() const {
   DCHECK(!upload_content_set_);
   DCHECK(upload_content_.empty());
   DCHECK(upload_file_path_.empty());
   DCHECK(upload_stream_factory_.is_null());
 }
 
 }  // namespace net