net: Move ownership of UploadDataStream from URLRequestHttpJob to URLRequest

net/url_request/url_request.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_request.h"
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/callback.h"
 #include "base/compiler_specific.h"
 #include "base/debug/stack_trace.h"
 #include "base/lazy_instance.h"
 #include "base/memory/singleton.h"
 #include "base/message_loop.h"
 #include "base/metrics/stats_counters.h"
 #include "base/stl_util.h"
 #include "base/synchronization/lock.h"
 #include "net/base/auth.h"
 #include "net/base/host_port_pair.h"
 #include "net/base/load_flags.h"
 #include "net/base/net_errors.h"
 #include "net/base/net_log.h"
 #include "net/base/network_change_notifier.h"
 #include "net/base/network_delegate.h"
 #include "net/base/ssl_cert_request_info.h"
 #include "net/base/upload_data.h"
+#include "net/base/upload_data_stream.h"
 #include "net/http/http_response_headers.h"
 #include "net/http/http_util.h"
 #include "net/url_request/url_request_context.h"
 #include "net/url_request/url_request_error_job.h"
 #include "net/url_request/url_request_job.h"
 #include "net/url_request/url_request_job_manager.h"
 #include "net/url_request/url_request_netlog_params.h"
 #include "net/url_request/url_request_redirect_job.h"
 
 using base::Time;
@@ skipping 98 matching lines @@
 // URLRequest
 
 // TODO(shalev): Get rid of this constructor in favour of the one below it.
 URLRequest::URLRequest(const GURL& url,
                        Delegate* delegate,
                        const URLRequestContext* context)
     : context_(context),
       network_delegate_(context->network_delegate()),
       net_log_(BoundNetLog::Make(context->net_log(),
                                  NetLog::SOURCE_URL_REQUEST)),
+      disable_upload_(false),
       url_chain_(1, url),
       method_("GET"),
       referrer_policy_(CLEAR_REFERRER_ON_TRANSITION_FROM_SECURE_TO_INSECURE),
       load_flags_(LOAD_NORMAL),
       delegate_(delegate),
       is_pending_(false),
       is_redirecting_(false),
       redirect_limit_(kMaxRedirects),
       priority_(LOWEST),
       identifier_(GenerateURLRequestIdentifier()),
@@ skipping 19 matching lines @@
 }
 
 URLRequest::URLRequest(const GURL& url,
                        Delegate* delegate,
                        const URLRequestContext* context,
                        NetworkDelegate* network_delegate)
     : context_(context),
       network_delegate_(network_delegate),
       net_log_(BoundNetLog::Make(context->net_log(),
                                  NetLog::SOURCE_URL_REQUEST)),
+      disable_upload_(false),
       url_chain_(1, url),
       method_("GET"),
       referrer_policy_(CLEAR_REFERRER_ON_TRANSITION_FROM_SECURE_TO_INSECURE),
       load_flags_(LOAD_NORMAL),
       delegate_(delegate),
       is_pending_(false),
       is_redirecting_(false),
       redirect_limit_(kMaxRedirects),
       priority_(LOWEST),
       identifier_(GenerateURLRequestIdentifier()),
@@ skipping 52 matching lines @@
 void URLRequest::RegisterRequestInterceptor(Interceptor* interceptor) {
   URLRequestJobManager::GetInstance()->RegisterRequestInterceptor(interceptor);
 }
 
 // static
 void URLRequest::UnregisterRequestInterceptor(Interceptor* interceptor) {
   URLRequestJobManager::GetInstance()->UnregisterRequestInterceptor(
       interceptor);
 }
 
-void URLRequest::AppendBytesToUpload(const char* bytes, int bytes_len) {
-  DCHECK(bytes_len > 0 && bytes);
-  if (!upload_)
-    upload_ = new UploadData();
-  upload_->AppendBytes(bytes, bytes_len);
-}
-
 void URLRequest::EnableChunkedUpload() {
   DCHECK(!upload_ || upload_->is_chunked());
   if (!upload_) {
     upload_ = new UploadData();
     upload_->set_is_chunked(true);
+    upload_data_stream_.reset(new UploadDataStream(upload_));
   }
 }
 
 void URLRequest::AppendChunkToUpload(const char* bytes,
                                      int bytes_len,
                                      bool is_last_chunk) {
   DCHECK(upload_);
   DCHECK(upload_->is_chunked());
   DCHECK_GT(bytes_len, 0);
   upload_->AppendChunk(bytes, bytes_len, is_last_chunk);
 }
 
 void URLRequest::set_upload(UploadData* upload) {
   upload_ = upload;
+  upload_data_stream_.reset(new UploadDataStream(upload_));
 }
 
-// Get the upload data directly.
-const UploadData* URLRequest::get_upload() const {
-  return upload_.get();
-}
-
-UploadData* URLRequest::get_upload_mutable() {
-  return upload_.get();
+const UploadDataStream* URLRequest::get_upload() const {

[mmenke, 2012/11/19 17:21:21]

For this function and the next, what if disable_upload_ is true?

[hashimoto, 2012/11/20 07:48:46]

Removed disable_upload_.

+  return upload_data_stream_.get();
 }
 
 bool URLRequest::has_upload() const {
-  return upload_ != NULL;
+  return upload_data_stream_.get() != NULL;
 }
 
 void URLRequest::SetExtraRequestHeaderById(int id, const string& value,
                                            bool overwrite) {
   DCHECK(!is_pending_ || is_redirecting_);
   NOTREACHED() << "implement me!";
 }
 
 void URLRequest::SetExtraRequestHeaderByName(const string& name,
                                              const string& value,
@@ skipping 218 matching lines @@
 
   net_log_.BeginEvent(
       NetLog::TYPE_URL_REQUEST_START_JOB,
       base::Bind(&NetLogURLRequestStartCallback,
                  &url(), &method_, load_flags_, priority_,
                  upload_.get() ? upload_->identifier() : -1));
 
   job_ = job;
   job_->SetExtraRequestHeaders(extra_request_headers_);
 
-  if (upload_.get())
-    job_->SetUpload(upload_.get());
+  if (upload_data_stream_.get() && !disable_upload_)
+    job_->SetUpload(upload_data_stream_.get());
 
   is_pending_ = true;
   is_redirecting_ = false;
 
   response_info_.was_cached = false;
 
   // Don't allow errors to be sent from within Start().
   // TODO(brettw) this may cause NotifyDone to be sent synchronously,
   // we probably don't want this: they should be sent asynchronously so
   // the caller does not get reentered.
@@ skipping 233 matching lines @@
   // be converted to GETs when following 301/302 redirects, for historical
   // reasons. Most major browsers do this and so shall we.  Both RFC 2616 and
   // the httpbis draft say to prompt the user to confirm the generation of new
   // requests, other than GET and HEAD requests, but IE omits these prompts and
   // so shall we.
   // See:  https://tools.ietf.org/html/draft-ietf-httpbis-p2-semantics-17#section-7.3
   bool was_post = method_ == "POST";
   if ((http_status_code == 303 && method_ != "HEAD") ||
       ((http_status_code == 301 || http_status_code == 302) && was_post)) {
     method_ = "GET";
-    upload_ = NULL;
+    disable_upload_ = true;

[mmenke, 2012/11/19 17:21:21]

Why do we need to keep the stream around?  Since we have a response, the entire
request has already been sent, so it should no longer be needed by the stream.

We already basically do the same thing when re-initializing the UploadStream -
any further access by the old stream would be a problem.

[hashimoto, 2012/11/20 07:48:46]

I thought it's good to disable the stream instead of discarding it because we
can reuse it, but I agree that no one needs to reuse it.
job_->GetUploadProgress() in line 803 accesses UploadDataStream so we should
keep it alive at least until then.
Also, since URLRequest::OrphanJob is not called until PrepareToRestart, I'd like
to keep the stream until then.
How does this new change look?

     if (was_post) {
       // If being switched from POST to GET, must remove headers that were
       // specific to the POST and don't have meaning in GET. For example
       // the inclusion of a multipart Content-Type header in GET can cause
       // problems with some servers:
       // http://code.google.com/p/chromium/issues/detail?id=843
       StripPostSpecificHeaders(&extra_request_headers_);
     }
   }
 
@@ skipping 188 matching lines @@
       new base::debug::StackTrace(NULL, 0);
   *stack_trace_copy = stack_trace;
   stack_trace_.reset(stack_trace_copy);
 }
 
 const base::debug::StackTrace* URLRequest::stack_trace() const {
   return stack_trace_.get();
 }
 
 }  // namespace net