Fix a pair of Cronet upload bugs and add tests.

When it retried uploads using Channels, it would restart the upload with
data from the middle of the file, since Channels can't be rewound.  It
now just fails those requests instead.

Also, it was impossible to set the method for uploads - the default
value (POST) would overwrite any method set by the embedder.

Adds a new set of upload tests using the embedded test server, as
uploads were basically untested before.  Chunked uploads are still not
being tested, as the test server doesn't support them yet.

BUG=424712

Review URL: https://codereview.chromium.org/640593004

Cr-Commit-Position: refs/heads/master@{#300316}

9 files changed, 442 insertions, 15 deletions

diff --git a/components/cronet.gypi b/components/cronet.gypi
index d33f982..ecf374c 100644
--- a/components/cronet.gypi
+++ b/components/cronet.gypi
@@ -412,6 +412,7 @@
           'sources': [
             'cronet/android/test/src/org/chromium/cronet_test_apk/CronetTestUtil.java',
             'cronet/android/test/src/org/chromium/cronet_test_apk/MockUrlRequestJobUtil.java',
+            'cronet/android/test/src/org/chromium/cronet_test_apk/UploadTestServer.java',
           ],
           'variables': {
             'jni_gen_package': 'cronet_tests',
@@ -425,6 +426,8 @@
             'cronet/android/test/cronet_test_jni.cc',
             'cronet/android/test/mock_url_request_job_util.cc',
             'cronet/android/test/mock_url_request_job_util.h',
+            'cronet/android/test/upload_test_server.cc',
+            'cronet/android/test/upload_test_server.h',
             '../net/base/directory_lister.cc',
             '../net/base/directory_lister.h',
             '../net/url_request/url_request_file_job.cc',
diff --git a/components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java b/components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java
index 9149781..eb0e575 100644
--- a/components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java
+++ b/components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java
@@ -319,19 +319,6 @@ public class ChromiumUrlRequest implements HttpUrlRequest {
 
             mStarted = true;
 
-            String method = mMethod;
-            if (method == null &&
-                    ((mUploadData != null && mUploadData.length > 0) ||
-                      mUploadChannel != null || mChunkedUpload)) {
-                // Default to POST if there is data to upload but no method was
-                // specified.
-                method = "POST";
-            }
-
-            if (method != null) {
-                nativeSetMethod(mUrlRequestAdapter, method);
-            }
-
             if (mHeaders != null && !mHeaders.isEmpty()) {
                 for (Entry<String, String> entry : mHeaders.entrySet()) {
                     nativeAddHeader(mUrlRequestAdapter, entry.getKey(),
@@ -358,6 +345,13 @@ public class ChromiumUrlRequest implements HttpUrlRequest {
                                           mUploadContentType);
             }
 
+            // Note:  The above functions to set the upload body also set the
+            // method to POST, behind the scenes, so if mMethod is null but
+            // there's an upload body, the method will default to POST.
+            if (mMethod != null) {
+                nativeSetMethod(mUrlRequestAdapter, mMethod);
+            }
+
             nativeStart(mUrlRequestAdapter);
         }
     }
diff --git a/components/cronet/android/test/cronet_test_jni.cc b/components/cronet/android/test/cronet_test_jni.cc
index dcade5e..3bbaa676 100644
--- a/components/cronet/android/test/cronet_test_jni.cc
+++ b/components/cronet/android/test/cronet_test_jni.cc
@@ -9,11 +9,13 @@
 #include "base/android/jni_registrar.h"
 #include "components/cronet/android/cronet_loader.h"
 #include "mock_url_request_job_util.h"
+#include "upload_test_server.h"
 
 namespace {
 
 const base::android::RegistrationMethod kCronetTestsRegisteredMethods[] = {
   {"MockURLRequestJobUtil", cronet::RegisterMockUrlRequestJobUtil},
+  {"RegisterUploadTestServer", cronet::RegisterUploadTestServer},
 };
 
 }  // namespace
diff --git a/components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/TestHttpUrlRequestListener.java b/components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/TestHttpUrlRequestListener.java
index 4f9db10..22429ae 100644
--- a/components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/TestHttpUrlRequestListener.java
+++ b/components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/TestHttpUrlRequestListener.java
@@ -17,10 +17,14 @@ import org.chromium.net.HttpUrlRequestListener;
 
 public class TestHttpUrlRequestListener implements HttpUrlRequestListener {
     public static final String TAG = "TestHttpUrlRequestListener";
+
     public int mHttpStatusCode = 0;
+    public String mNegotiatedProtocol;
+
     public String mUrl;
     public byte[] mResponseAsBytes;
-    public String mNegotiatedProtocol;
+    public String mResponseAsString;
+    public Exception mException;
 
     private ConditionVariable mComplete = new ConditionVariable();
 
@@ -40,6 +44,8 @@ public class TestHttpUrlRequestListener implements HttpUrlRequestListener {
     public void onRequestComplete(HttpUrlRequest request) {
         mUrl = request.getUrl();
         mResponseAsBytes = request.getResponseAsBytes();
+        mResponseAsString = new String(mResponseAsBytes);
+        mException = request.getException();
         mComplete.open();
         Log.i(TAG, "****** Request Complete, status code is " +
                 request.getHttpStatusCode());
diff --git a/components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/UploadTest.java b/components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/UploadTest.java
new file mode 100644
index 0000000..959aac2
--- /dev/null
+++ b/components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/UploadTest.java
@@ -0,0 +1,237 @@
+// Copyright 2014 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.
+
+package org.chromium.cronet_test_apk;
+
+import android.test.suitebuilder.annotation.SmallTest;
+
+import org.chromium.base.test.util.Feature;
+import org.chromium.net.HttpUrlRequest;
+import org.chromium.net.HttpUrlRequestListener;
+
+import java.io.ByteArrayInputStream;
+import java.io.InputStream;
+import java.nio.channels.Channels;
+import java.nio.channels.ReadableByteChannel;
+import java.util.HashMap;
+
+/**
+ * Test fixture to test upload APIs.  Uses an in-process test server.
+ */
+public class UploadTest extends CronetTestBase {
+    private static final String UPLOAD_DATA = "Nifty upload data!";
+    private static final String UPLOAD_CHANNEL_DATA = "Upload channel data";
+
+    private CronetTestActivity mActivity;
+
+    // @Override
+    protected void setUp() throws Exception {
+        super.setUp();
+        mActivity = launchCronetTestApp();
+        assertNotNull(mActivity);
+        assertTrue(UploadTestServer.startUploadTestServer());
+    }
+
+    private HttpUrlRequest createRequest(
+            String url, HttpUrlRequestListener listener) {
+        HashMap<String, String> headers = new HashMap<String, String>();
+        return mActivity.mRequestFactory.createRequest(
+                url, HttpUrlRequest.REQUEST_PRIORITY_MEDIUM, headers, listener);
+    }
+
+    /**
+     * Sets request to have an upload channel containing the given data.
+     * uploadDataLength should generally be uploadData.length(), unless a test
+     * needs to get a read error.
+     */
+    private void setUploadChannel(HttpUrlRequest request,
+                                  String contentType,
+                                  String uploadData,
+                                  int uploadDataLength) {
+        InputStream uploadDataStream = new ByteArrayInputStream(
+                uploadData.getBytes());
+        ReadableByteChannel uploadDataChannel =
+                Channels.newChannel(uploadDataStream);
+        request.setUploadChannel(
+                contentType, uploadDataChannel, uploadDataLength);
+    }
+
+    /**
+     * Tests uploading an in-memory string.
+     */
+    @SmallTest
+    @Feature({"Cronet"})
+    public void testUploadData() throws Exception {
+        TestHttpUrlRequestListener listener = new TestHttpUrlRequestListener();
+        HttpUrlRequest request = createRequest(
+                UploadTestServer.getEchoBodyURL(), listener);
+        request.setUploadData("text/plain", UPLOAD_DATA.getBytes("UTF8"));
+        request.start();
+        listener.blockForComplete();
+
+        assertEquals(200, listener.mHttpStatusCode);
+        assertEquals(UPLOAD_DATA, listener.mResponseAsString);
+    }
+
+    /**
+     * Tests uploading an in-memory string with a redirect that preserves the
+     * POST body.  This makes sure the body is correctly sent again.
+     */
+    @SmallTest
+    @Feature({"Cronet"})
+    public void testUploadDataWithRedirect() throws Exception {
+        TestHttpUrlRequestListener listener = new TestHttpUrlRequestListener();
+        HttpUrlRequest request = createRequest(
+                UploadTestServer.getRedirectToEchoBody(), listener);
+        request.setUploadData("text/plain", UPLOAD_DATA.getBytes("UTF8"));
+        request.start();
+        listener.blockForComplete();
+
+        assertEquals(200, listener.mHttpStatusCode);
+        assertEquals(UPLOAD_DATA, listener.mResponseAsString);
+    }
+
+    /**
+     * Tests Content-Type can be set when uploading an in-memory string.
+     */
+    @SmallTest
+    @Feature({"Cronet"})
+    public void testUploadDataContentType() throws Exception {
+        String contentTypes[] = {"text/plain", "chicken/spicy"};
+        for (String contentType : contentTypes) {
+            TestHttpUrlRequestListener listener =
+                    new TestHttpUrlRequestListener();
+            HttpUrlRequest request = createRequest(
+                    UploadTestServer.getEchoHeaderURL("Content-Type"),
+                    listener);
+            request.setUploadData(contentType, UPLOAD_DATA.getBytes("UTF8"));
+            request.start();
+            listener.blockForComplete();
+
+            assertEquals(200, listener.mHttpStatusCode);
+            assertEquals(contentType, listener.mResponseAsString);
+        }
+    }
+
+    /**
+     * Tests the default method when uploading.
+     */
+    @SmallTest
+    @Feature({"Cronet"})
+    public void testDefaultUploadMethod() throws Exception {
+        TestHttpUrlRequestListener listener = new TestHttpUrlRequestListener();
+        HttpUrlRequest request = createRequest(
+                UploadTestServer.getEchoMethodURL(), listener);
+        request.setUploadData("text/plain", UPLOAD_DATA.getBytes("UTF8"));
+        request.start();
+        listener.blockForComplete();
+
+        assertEquals(200, listener.mHttpStatusCode);
+        assertEquals("POST", listener.mResponseAsString);
+    }
+
+    /**
+     * Tests methods can be set when uploading.
+     */
+    @SmallTest
+    @Feature({"Cronet"})
+    public void testUploadMethods() throws Exception {
+        String uploadMethods[] = {"POST", "PUT"};
+        for (String uploadMethod : uploadMethods) {
+            TestHttpUrlRequestListener listener =
+                    new TestHttpUrlRequestListener();
+            HttpUrlRequest request = createRequest(
+                    UploadTestServer.getEchoMethodURL(), listener);
+            request.setHttpMethod(uploadMethod);
+            request.setUploadData("text/plain", UPLOAD_DATA.getBytes("UTF8"));
+            request.start();
+            listener.blockForComplete();
+
+            assertEquals(200, listener.mHttpStatusCode);
+            assertEquals(uploadMethod, listener.mResponseAsString);
+        }
+    }
+
+    /**
+     * Tests uploading from a channel.
+     */
+    @SmallTest
+    @Feature({"Cronet"})
+    public void testUploadChannel() throws Exception {
+        TestHttpUrlRequestListener listener = new TestHttpUrlRequestListener();
+        HttpUrlRequest request = createRequest(
+                UploadTestServer.getEchoBodyURL(), listener);
+        setUploadChannel(request, "text/plain", UPLOAD_CHANNEL_DATA,
+                         UPLOAD_CHANNEL_DATA.length());
+        request.start();
+        listener.blockForComplete();
+
+        assertEquals(200, listener.mHttpStatusCode);
+        assertEquals(UPLOAD_CHANNEL_DATA, listener.mResponseAsString);
+    }
+
+    /**
+     * Tests uploading from a channel in the case a redirect preserves the post
+     * body.  Since channels can't be rewound, the request fails when we try to
+     * rewind it to send the second request.
+     */
+    @SmallTest
+    @Feature({"Cronet"})
+    public void testUploadChannelWithRedirect() throws Exception {
+        TestHttpUrlRequestListener listener = new TestHttpUrlRequestListener();
+        HttpUrlRequest request = createRequest(
+                UploadTestServer.getRedirectToEchoBody(), listener);
+        setUploadChannel(request, "text/plain", UPLOAD_CHANNEL_DATA,
+                         UPLOAD_CHANNEL_DATA.length());
+        request.start();
+        listener.blockForComplete();
+
+        assertEquals(0, listener.mHttpStatusCode);
+        assertEquals(
+                "System error: net::ERR_UPLOAD_STREAM_REWIND_NOT_SUPPORTED(-25)",
+                listener.mException.getMessage());
+    }
+
+    /**
+     * Tests uploading from a channel when there's a read error.  The body
+     * should be 0-padded.
+     */
+    @SmallTest
+    @Feature({"Cronet"})
+    public void testUploadChannelWithReadError() throws Exception {
+        TestHttpUrlRequestListener listener = new TestHttpUrlRequestListener();
+        HttpUrlRequest request = createRequest(
+                UploadTestServer.getEchoBodyURL(), listener);
+        setUploadChannel(request, "text/plain", UPLOAD_CHANNEL_DATA,
+                         UPLOAD_CHANNEL_DATA.length() + 2);
+        request.start();
+        listener.blockForComplete();
+
+        assertEquals(200, listener.mHttpStatusCode);
+        assertEquals(UPLOAD_CHANNEL_DATA + "\0\0", listener.mResponseAsString);
+    }
+
+    /**
+     * Tests Content-Type can be set when uploading from a channel.
+     */
+    @SmallTest
+    @Feature({"Cronet"})
+    public void testUploadChannelContentType() throws Exception {
+        String contentTypes[] = {"text/plain", "chicken/spicy"};
+        for (String contentType : contentTypes) {
+            TestHttpUrlRequestListener listener =
+                    new TestHttpUrlRequestListener();
+            HttpUrlRequest request = createRequest(
+                    UploadTestServer.getEchoHeaderURL("Content-Type"),
+                                                      listener);
+            setUploadChannel(request, contentType, UPLOAD_CHANNEL_DATA,
+                             UPLOAD_CHANNEL_DATA.length());
+            request.start();
+            listener.blockForComplete();
+
+            assertEquals(200, listener.mHttpStatusCode);
+            assertEquals(contentType, listener.mResponseAsString);
+        }
+    }
+}
diff --git a/components/cronet/android/test/src/org/chromium/cronet_test_apk/UploadTestServer.java b/components/cronet/android/test/src/org/chromium/cronet_test_apk/UploadTestServer.java
new file mode 100644
index 0000000..8d24887
--- /dev/null
+++ b/components/cronet/android/test/src/org/chromium/cronet_test_apk/UploadTestServer.java
@@ -0,0 +1,45 @@
+// Copyright 2014 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.
+
+package org.chromium.cronet_test_apk;
+
+import org.chromium.base.JNINamespace;
+
+/**
+ * Wrapper class to start an in-process native test server, and get URLs
+ * needed to talk to it.
+ */
+@JNINamespace("cronet")
+public final class UploadTestServer {
+    public static boolean startUploadTestServer() {
+        return nativeStartUploadTestServer();
+    }
+
+    public static void shutdownUploadTestServer() {
+        nativeShutdownUploadTestServer();
+    }
+
+    public static String getEchoBodyURL() {
+        return nativeGetEchoBodyURL();
+    }
+
+    public static String getEchoHeaderURL(String header) {
+        return nativeGetEchoHeaderURL(header);
+    }
+
+    public static String getEchoMethodURL() {
+        return nativeGetEchoMethodURL();
+    }
+
+    public static String getRedirectToEchoBody() {
+        return nativeGetRedirectToEchoBody();
+    }
+
+    private static native boolean nativeStartUploadTestServer();
+    private static native void nativeShutdownUploadTestServer();
+    private static native String nativeGetEchoBodyURL();
+    private static native String nativeGetEchoHeaderURL(String header);
+    private static native String nativeGetEchoMethodURL();
+    private static native String nativeGetRedirectToEchoBody();
+}
diff --git a/components/cronet/android/test/upload_test_server.cc b/components/cronet/android/test/upload_test_server.cc
new file mode 100644
index 0000000..1c7b8c8
--- /dev/null
+++ b/components/cronet/android/test/upload_test_server.cc
@@ -0,0 +1,123 @@
+// Copyright 2014 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 "upload_test_server.h"
+
+#include "base/android/jni_android.h"
+#include "base/android/jni_string.h"
+#include "base/bind.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/strings/string_util.h"
+#include "jni/UploadTestServer_jni.h"
+#include "net/http/http_status_code.h"
+#include "net/test/embedded_test_server/embedded_test_server.h"
+#include "net/test/embedded_test_server/http_request.h"
+#include "net/test/embedded_test_server/http_response.h"
+#include "url/gurl.h"
+
+namespace cronet {
+
+namespace {
+
+const char echo_body_path[] = "/echo_body";
+const char echo_header_path[] = "/echo_header";
+const char echo_method_path[] = "/echo_method";
+const char redirect_to_echo_body_path[] = "/redirect_to_echo_body";
+
+net::test_server::EmbeddedTestServer* g_test_server = nullptr;
+
+scoped_ptr<net::test_server::HttpResponse> UploadServerRequestHandler(
+    const net::test_server::HttpRequest& request) {
+  DCHECK(g_test_server);
+  scoped_ptr<net::test_server::BasicHttpResponse> response(
+      new net::test_server::BasicHttpResponse());
+  response->set_content_type("text/plain");
+
+  if (request.relative_url == echo_body_path) {
+    if (request.has_content) {
+      response->set_content(request.content);
+    } else {
+      response->set_content("Request has no body. :(");
+    }
+    return response.Pass();
+  }
+
+  if (StartsWithASCII(request.relative_url, echo_header_path, true)) {
+    GURL url = g_test_server->GetURL(request.relative_url);
+    auto it = request.headers.find(url.query());
+    if (it != request.headers.end()) {
+      response->set_content(it->second);
+    } else {
+      response->set_content("Header not found. :(");
+    }
+    return response.Pass();
+  }
+
+  if (request.relative_url == echo_method_path) {
+    response->set_content(request.method_string);
+    return response.Pass();
+  }
+
+  if (request.relative_url == redirect_to_echo_body_path) {
+    response->set_code(net::HTTP_TEMPORARY_REDIRECT);
+    response->AddCustomHeader("Location", echo_body_path);
+    return response.Pass();
+  }
+
+  // Unhandled requests result in the Embedded test server sending a 404.
+  return scoped_ptr<net::test_server::BasicHttpResponse>();
+}
+
+}  // namespace
+
+jboolean StartUploadTestServer(JNIEnv* env, jclass jcaller) {
+  // Shouldn't happen.
+  if (g_test_server)
+    return false;
+  g_test_server = new net::test_server::EmbeddedTestServer();
+  g_test_server->RegisterRequestHandler(
+      base::Bind(&UploadServerRequestHandler));
+  return g_test_server->InitializeAndWaitUntilReady();
+}
+
+void ShutdownUploadTestServer(JNIEnv* env, jclass jcaller) {
+  if (!g_test_server)
+    return;
+  delete g_test_server;
+  g_test_server = NULL;
+}
+
+jstring GetEchoBodyURL(JNIEnv* env, jclass jcaller) {
+  DCHECK(g_test_server);
+  GURL url = g_test_server->GetURL(echo_body_path);
+  return base::android::ConvertUTF8ToJavaString(env, url.spec()).Release();
+}
+
+jstring GetEchoHeaderURL(JNIEnv* env, jclass jcaller, jstring jheader) {
+  DCHECK(g_test_server);
+  GURL url = g_test_server->GetURL(echo_header_path);
+  GURL::Replacements replacements;
+  std::string header = base::android::ConvertJavaStringToUTF8(env, jheader);
+  replacements.SetQueryStr(header.c_str());
+  url = url.ReplaceComponents(replacements);
+  return base::android::ConvertUTF8ToJavaString(env, url.spec()).Release();
+}
+
+jstring GetEchoMethodURL(JNIEnv* env, jclass jcaller) {
+  DCHECK(g_test_server);
+  GURL url = g_test_server->GetURL(echo_method_path);
+  return base::android::ConvertUTF8ToJavaString(env, url.spec()).Release();
+}
+
+jstring GetRedirectToEchoBody(JNIEnv* env, jclass jcaller) {
+  DCHECK(g_test_server);
+  GURL url = g_test_server->GetURL(redirect_to_echo_body_path);
+  return base::android::ConvertUTF8ToJavaString(env, url.spec()).Release();
+}
+
+bool RegisterUploadTestServer(JNIEnv* env) {
+  return RegisterNativesImpl(env);
+}
+
+}  // namespace cronet
diff --git a/components/cronet/android/test/upload_test_server.h b/components/cronet/android/test/upload_test_server.h
new file mode 100644
index 0000000..59eb45c
--- /dev/null
+++ b/components/cronet/android/test/upload_test_server.h
@@ -0,0 +1,16 @@
+// Copyright 2014 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.
+
+#ifndef CRONET_UPLOAD_TEST_SERVER_H_
+#define CRONET_UPLOAD_TEST_SERVER_H_
+
+#include <jni.h>
+
+namespace cronet {
+
+bool RegisterUploadTestServer(JNIEnv* env);
+
+}  // namespace cronet
+
+#endif  // CRONET_UPLOAD_TEST_SERVER_H_
diff --git a/components/cronet/android/wrapped_channel_upload_element_reader.cc b/components/cronet/android/wrapped_channel_upload_element_reader.cc
index a4b0d0d..750d382 100644
--- a/components/cronet/android/wrapped_channel_upload_element_reader.cc
+++ b/components/cronet/android/wrapped_channel_upload_element_reader.cc
@@ -21,7 +21,8 @@ WrappedChannelElementReader::~WrappedChannelElementReader() {
 }
 
 int WrappedChannelElementReader::Init(const net::CompletionCallback& callback) {
-  offset_ = 0;
+  if (offset_ != 0)
+    return net::ERR_UPLOAD_STREAM_REWIND_NOT_SUPPORTED;
   return net::OK;
 }