Prevent URLFetcher::AppendChunkedData from dereferencing NULL pointers.

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 187 matching lines @@
 
   upload_content_type_ = content_type;
   upload_content_.clear();
   is_chunked_upload_ = true;
 }
 
 void URLFetcherCore::AppendChunkToUpload(const std::string& content,
                                          bool is_last_chunk) {
   DCHECK(delegate_task_runner_.get());
   DCHECK(network_task_runner_.get());
+  DCHECK(is_chunked_upload_);
   network_task_runner_->PostTask(
       FROM_HERE,
       base::Bind(&URLFetcherCore::CompleteAddingUploadDataChunk, this, content,
                  is_last_chunk));
 }
 
 void URLFetcherCore::SetLoadFlags(int load_flags) {
   load_flags_ = load_flags;
 }
 
@@ skipping 291 matching lines @@
 
 URLFetcherCore::~URLFetcherCore() {
   // |request_| should be NULL. If not, it's unsafe to delete it here since we
   // may not be on the IO thread.
   DCHECK(!request_.get());
 }
 
 void URLFetcherCore::StartOnIOThread() {
   DCHECK(network_task_runner_->BelongsToCurrentThread());
 
+  // Create ChunkedUploadDataStream, if needed, so the consumer can start
+  // appending data.  Have to do it here because StartURLRequest() may be called
+  // asynchonously.
+  if (is_chunked_upload_) {

[eroman, 2016/03/22 19:05:37]

What is the gap between when this runs, and when StartURLRequest() runs?

Does this actually need to be set up here, will users be able to append data to
chunked_stream_ before it is moved to the URLRequest ?

This seems weird in that ChunkedUploadDataStream() is itself just a buffer that
queues up the data. So we have a writer interface to ChunkedUploadDataStream,
and then URLRequest pulls data from that.

I will have to dig deeper into how we do the upload abstractions to comment
better on this, although your feedback will speed that up :)

[mmenke, 2016/03/22 20:01:51]

There are two gaps:  The ResponseWriter may take a while to initialize (Open +
create file, or whatever), and in DidInitializeWriter ->
StartURLRequestWhenAppropriate, we may delay the request based on the
ThrottleManager (A class I would love to kill).

We could move URLRequest creation up here, and have the URLRequest exist for the
duration of both those things.  I'm fine with doing that refactor in this CL.

+    chunked_stream_.reset(new ChunkedUploadDataStream(0));
+    chunked_stream_writer_ = chunked_stream_->CreateWriter();
+  }
+
   if (!response_writer_)
     response_writer_.reset(new URLFetcherStringWriter);
 
   const int result = response_writer_->Initialize(
       base::Bind(&URLFetcherCore::DidInitializeWriter, this));
   if (result != ERR_IO_PENDING)
     DidInitializeWriter(result);
 }
 
 void URLFetcherCore::StartURLRequest() {
@@ skipping 14 matching lines @@
   DCHECK(!request_.get());
 
   g_registry.Get().AddURLFetcherCore(this);
   current_response_bytes_ = 0;
   request_context_getter_->AddObserver(this);
   request_ = request_context_getter_->GetURLRequestContext()->CreateRequest(
       original_url_, DEFAULT_PRIORITY, this);
   request_->set_stack_trace(stack_trace_);
   int flags = request_->load_flags() | load_flags_;
 
-  if (is_chunked_upload_)
-    request_->EnableChunkedUpload();
+  // TODO(mmenke): This should really be with the other code to set the upload
+  // body, below.
+  if (chunked_stream_)
+    request_->set_upload(std::move(chunked_stream_));
+
   request_->SetLoadFlags(flags);
   request_->SetReferrer(referrer_);
   request_->set_referrer_policy(referrer_policy_);
   request_->set_first_party_for_cookies(initiator_.is_empty() ? original_url_
                                                               : initiator_);
   request_->set_initiator(initiator_.is_empty() ? url::Origin(original_url_)
                                                 : url::Origin(initiator_));
   if (url_request_data_key_ && !url_request_create_data_callback_.is_null()) {
     request_->SetUserData(url_request_data_key_,
                           url_request_create_data_callback_.Run());
@@ skipping 263 matching lines @@
     destination_url_backoff =
         url_throttler_entry_->GetExponentialBackoffReleaseTime();
   }
 
   return original_url_backoff > destination_url_backoff ?
       original_url_backoff : destination_url_backoff;
 }
 
 void URLFetcherCore::CompleteAddingUploadDataChunk(
     const std::string& content, bool is_last_chunk) {
-  if (was_cancelled_) {
-    // Since CompleteAddingUploadDataChunk() is posted as a *delayed* task, it
-    // may run after the URLFetcher was already stopped.
-    return;
-  }
   DCHECK(is_chunked_upload_);
-  DCHECK(request_.get());
   DCHECK(!content.empty());
-  request_->AppendChunkToUpload(content.data(),
-                                static_cast<int>(content.length()),
-                                is_last_chunk);
+  chunked_stream_writer_->AppendData(
+      content.data(), static_cast<int>(content.length()), is_last_chunk);
 }
 
 int URLFetcherCore::WriteBuffer(scoped_refptr<DrainableIOBuffer> data) {
   while (data->BytesRemaining() > 0) {
     const int result = response_writer_->Write(
         data.get(),
         data->BytesRemaining(),
         base::Bind(&URLFetcherCore::DidWriteBuffer, this, data));
     if (result < 0) {
       if (result != ERR_IO_PENDING)
@@ skipping 94 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