Fix UrlFetcher upload retry handling.

net/url_request/url_fetcher_core.cc

Index: net/url_request/url_fetcher_core.cc
diff --git a/net/url_request/url_fetcher_core.cc b/net/url_request/url_fetcher_core.cc
index 016ddcb4d09064f13e5e1026086611ccfd795532..80ad4e69fe024d11d7450db75b02c57ce01cdeed 100644
--- a/net/url_request/url_fetcher_core.cc
+++ b/net/url_request/url_fetcher_core.cc
@@ -331,44 +331,42 @@ void URLFetcherCore::Stop() {
 void URLFetcherCore::SetUploadData(const std::string& upload_content_type,
                                    const std::string& upload_content) {
   DCHECK(!is_chunked_upload_);
-  DCHECK(!upload_content_);
-  DCHECK(upload_content_type_.empty());
-
-  // Empty |upload_content_type| is allowed iff the |upload_content| is empty.
-  DCHECK(upload_content.empty() || !upload_content_type.empty());
-
+  DCHECK(!upload_data_stream_);
   upload_content_type_ = upload_content_type;
-  scoped_ptr<UploadElementReader> reader(
-      UploadOwnedBytesElementReader::CreateWithString(upload_content));
-  upload_content_.reset(UploadDataStream::CreateWithReader(reader.Pass(), 0));
+  upload_content_ = upload_content;
 }
 
 void URLFetcherCore::SetUploadDataStream(
     const std::string& upload_content_type,
-    scoped_ptr<UploadDataStream> upload_content) {
+    scoped_ptr<UploadDataStream> upload_data_stream) {
   DCHECK(!is_chunked_upload_);
-  DCHECK(!upload_content_);
+  DCHECK(upload_content_.empty());
+  DCHECK(!upload_data_stream_);
   DCHECK(upload_content_type_.empty());
 
+  // Data stream uploading can't be retried, as the stream can't be restarted.
+  DCHECK(!automatically_retry_on_5xx_ || max_retries_on_5xx_ == 0);
+  DCHECK_EQ(max_retries_on_network_changes_, 0);
+
   // Empty |upload_content_type| is not allowed here, because it is impossible
   // to ensure non-empty |upload_content| as it may not be initialized yet.
   DCHECK(!upload_content_type.empty());
 
   upload_content_type_ = upload_content_type;
-  upload_content_ = upload_content.Pass();
+  upload_data_stream_ = upload_data_stream.Pass();
 }
 
 void URLFetcherCore::SetChunkedUpload(const std::string& content_type) {
   DCHECK(is_chunked_upload_ ||
          (upload_content_type_.empty() &&
-          !upload_content_));
+          upload_content_.empty()));
 
   // Empty |content_type| is not allowed here, because it is impossible
   // to ensure non-empty upload content as it is not yet supplied.
   DCHECK(!content_type.empty());
 
   upload_content_type_ = content_type;
-  upload_content_.reset();
+  upload_content_.clear();
   is_chunked_upload_ = true;
 }
 
@@ -750,9 +748,6 @@ void URLFetcherCore::StartURLRequest() {
     case URLFetcher::POST:
     case URLFetcher::PUT:
     case URLFetcher::PATCH:
-      // Upload content must be set.
-      DCHECK(is_chunked_upload_ || upload_content_);
-
       request_->set_method(
           request_type_ == URLFetcher::POST ? "POST" :
           request_type_ == URLFetcher::PUT ? "PUT" : "PATCH");
@@ -760,8 +755,16 @@ void URLFetcherCore::StartURLRequest() {
         extra_request_headers_.SetHeader(HttpRequestHeaders::kContentType,
                                          upload_content_type_);
       }
-      if (upload_content_)
-        request_->set_upload(upload_content_.Pass());
+      if (!upload_content_.empty() || upload_content_type_.empty()) {
+        DCHECK(!upload_data_stream_);
+        scoped_ptr<UploadElementReader> reader(new UploadBytesElementReader(
+            upload_content_.data(), upload_content_.size()));
+        request_->set_upload(make_scoped_ptr(
+            UploadDataStream::CreateWithReader(reader.Pass(), 0)));
+      } else if (upload_data_stream_) {
+        request_->set_upload(upload_data_stream_.Pass());
+      }

[mmenke, 2013/02/11 17:31:35]

This isn't right in the chunked upload case, I believe.  In that case, we don't
want to call set_upload.

Also, retrying in the chunked upload case doesn't work at all.

[rmsousa, 2013/02/11 22:57:54]

Chunked uploads aren't allowed to have empty content types, so they shouldn't
get into this condition. Of course, DCHECKing it doesn't hurt.

[mmenke, 2013/02/11 23:05:01]

set_upload should never be called for chunked upload, but this code does, which
is incorrect.

[rmsousa, 2013/02/12 00:18:28]

What I meant is that the DCHECKS in the SetUpload* functions imply that chunked
uploads always have non-empty upload_content_type and empty
upload_content_upload and _data_stream_, so they will never fit the conditions
in the if/else if. But yeah, we can always make the check explicit.

+
       current_upload_bytes_ = -1;
       // TODO(kinaba): http://crbug.com/118103. Implement upload callback in the
       //  layer and avoid using timer here.