net: Move ownership of UploadDataStream from URLRequestHttpJob to URLRequest

net/url_request/url_request.cc

Index: net/url_request/url_request.cc
diff --git a/net/url_request/url_request.cc b/net/url_request/url_request.cc
index 7bfa77239054f02df0fa4003a2f0d2c61c8a4f5f..040c67072327af6fc5602dded21b0d9c088c142d 100644
--- a/net/url_request/url_request.cc
+++ b/net/url_request/url_request.cc
@@ -24,6 +24,7 @@
 #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"
@@ -142,6 +143,7 @@ URLRequest::URLRequest(const GURL& url,
       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),
@@ -181,6 +183,7 @@ URLRequest::URLRequest(const GURL& url,
       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),
@@ -253,18 +256,12 @@ void URLRequest::UnregisterRequestInterceptor(Interceptor* interceptor) {
       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_));
   }
 }
 
@@ -279,19 +276,15 @@ void URLRequest::AppendChunkToUpload(const char* bytes,
 
 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,
@@ -530,8 +523,8 @@ void URLRequest::StartJob(URLRequestJob* job) {
   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;
@@ -785,7 +778,7 @@ int URLRequest::Redirect(const GURL& location, int http_status_code) {
   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