Cronet modifications to support AGSA.

components/cronet/android/java/src/org/chromium/net/UrlRequest.java

Index: components/cronet/android/java/src/org/chromium/net/UrlRequest.java
diff --git a/components/cronet/android/java/src/org/chromium/net/UrlRequest.java b/components/cronet/android/java/src/org/chromium/net/UrlRequest.java
index 73e9abac4030dd20e16712b8e4dad5d8ea893d22..4b3b389a770ee82d49c3e1b278612f72284865ec 100644
--- a/components/cronet/android/java/src/org/chromium/net/UrlRequest.java
+++ b/components/cronet/android/java/src/org/chromium/net/UrlRequest.java
@@ -1,13 +1,15 @@
 // 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.net;
 
+import android.os.ConditionVariable;
+
 import org.apache.http.conn.ConnectTimeoutException;
 import org.chromium.base.CalledByNative;
 import org.chromium.base.JNINamespace;
 
 import java.io.IOException;
 import java.net.MalformedURLException;
 import java.net.URL;
@@ -35,24 +37,24 @@ public class UrlRequest {
     private final Map<String, String> mHeaders;
     private final WritableByteChannel mSink;
     private Map<String, String> mAdditionalHeaders;
     private String mUploadContentType;
     private String mMethod;
     private byte[] mUploadData;
     private ReadableByteChannel mUploadChannel;
-    private WritableByteChannel mOutputChannel;
     private IOException mSinkException;
     private volatile boolean mStarted;
     private volatile boolean mCanceled;
     private volatile boolean mRecycled;
     private volatile boolean mFinished;
     private boolean mHeadersAvailable;
     private String mContentType;
     private long mContentLength;
     private long mUploadContentLength;
+    private ConditionVariable mAppendChunkCondition;
     private final ContextLock mLock;
 
     /**
      * Native adapter object, owned by UrlRequest.
      */
     private long mUrlRequestAdapter;
 
@@ -106,14 +108,15 @@ public class UrlRequest {
      */
     public void setUploadData(String contentType, byte[] data) {
         synchronized (mLock) {
             validateNotStarted();
             mUploadContentType = contentType;
             mUploadData = data;
             mUploadChannel = null;
+            mAppendChunkCondition = null;
         }
     }
 
     /**
      * Sets a readable byte channel to upload as part of a POST request.
      *
      * @param contentType MIME type of the post content or null if this is not a
@@ -126,17 +129,75 @@ public class UrlRequest {
             ReadableByteChannel channel, long contentLength) {
         synchronized (mLock) {
             validateNotStarted();
             mUploadContentType = contentType;
             mUploadChannel = channel;
             mUploadContentLength = contentLength;
             mUploadData = null;
+            mAppendChunkCondition = null;
+        }
+    }
+
+    /**
+     * Sets this request up for chunked uploading. To upload data call
+     * {@link #appendChunkBlocking(ByteBuffer, boolean)} after {@link #start()}.
+     *
+     * @param contentType MIME type of the post content or null if this is not a
+     *            POST request.
+     */
+    public void setChunkedUpload(String contentType) {

[mmenke, 2014/08/15 15:30:21]

setUploadChannel uses chunked uploads, too, so should probably rename this.

[mef, 2014/08/15 15:42:52]

Actually, I think (correct me if I'm wrong) setUploadChannel does not use
chunked uploads any more. That was a change upstreamed back in July, which has
removed chunked uploads.

[mmenke, 2014/08/15 15:49:01]

You're right, was forgetting it took a length.

+        synchronized (mLock) {
+            validateNotStarted();
+            mUploadContentType = contentType;
+            mAppendChunkCondition = new ConditionVariable();
+            mUploadData = null;
+            mUploadChannel = null;
         }
     }
 
+    /**
+     * Uploads a new chunk. Must have called {@link #setChunkedUpload(String)}
+     * and {@link #start()}.
+     *
+     * @param chunk The data, which will not be modified. It must not be empty
+     *            and its current position must be zero.
+     * @param isLastChunk Whether this chunk is the last one.
+     */
+    // Invokes {@link #nativeAppendChunk(long, ByteBuffer, int, boolean)} and
+    // waits for it
+    // to notify completion.

[mmenke, 2014/08/15 15:30:21]

This should be merged with the above comment, I think...At least I've never see
two comment styles mixed like this.

[mdumitrescu, 2014/08/15 15:50:20]

Well, the implementation details shouldn't be in the javadoc (unless relevant).
Removed since it's pretty clear from the doc.

+    public void appendChunkBlocking(ByteBuffer chunk, boolean isLastChunk)
+            throws IOException {
+        if (!chunk.hasRemaining()) {
+            throw new IllegalArgumentException(
+                    "Attempted to write empty buffer.");
+        }
+        if (chunk.position() != 0) {
+            throw new IllegalArgumentException("The position must be zero.");
+        }
+        synchronized (mLock) {
+            if (!mStarted) {
+                throw new IllegalStateException("Request not yet started.");
+            }
+            if (mAppendChunkCondition == null) {
+                throw new IllegalStateException(
+                        "Request not set for chunked uploadind.");
+            }
+            if (mUrlRequestAdapter == 0) {
+                throw new IOException("Native peer destroyed.");
+            }
+            mAppendChunkCondition.close();
+            nativeAppendChunk(mUrlRequestAdapter, chunk, chunk.limit(),
+                    isLastChunk);
+        }
+        // Wait for the data to be actually consumed. Outside mLock to avoid
+        // deadlock.
+        mAppendChunkCondition.block();

[mmenke, 2014/08/15 15:30:21]

We're blocking on another thread to avoid a string copy?  Does this really make
sense?  I suppose these are rather large chunks?

[mef, 2014/08/15 15:42:52]

I think the idea is that one thread is writing into buffer and another (network
thread) is uploading from buffer, hence the lock. nativeAppendChunk posts to
network thread.

[mmenke, 2014/08/15 15:49:01]

Right...but the question is if two copies is worse than blocking on whatever
else the network thread is doing and a copy.  In general, I am very much against
blocking on another thread.

[mdumitrescu, 2014/08/15 15:50:20]

I'm very open to suggestions regarding this.

A copy wouldn't necessarily be a problem (well, unless we'd have to
allocateDirect). In my mind, the problem comes from url_request_adapter.cc.
Specifically:

void URLRequestAdapter::AppendChunk(const char* bytes, int bytes_len,
                                    bool is_last_chunk) {
  VLOG(1) << "AppendChunk, len: " << bytes_len << ", last: " << is_last_chunk;
  context_->GetNetworkTaskRunner()->PostTask(
      FROM_HERE,
      base::Bind(&URLRequestAdapter::OnAppendChunk,
                 base::Unretained(this),
                 bytes,
                 bytes_len,
                 is_last_chunk));
}

Since this sends the data to another thread what happens if instead we return
immediately, then call another appendChunk? Could the data become reordered? Or
what happens if the cancel() or getHttpStatusCode() is called? Are these cases
safe? If so, awesome, I'm all for making a copy right before
context_->GetNetworkTaskRunner()->PostTask() and returning immediately.

[mmenke, 2014/08/15 16:09:24]

Tasks are executed in the order they're posted, since the NetworkTaskRunner is a
SingleThreadedTaskRunner - a lot of things would break if that changed.

cancel() is also safe.  The upload data stream already has to handle both the
case where it receives data and the network request isn't ready to read it, and
where it's being used on cancellation.

getHttpStatusCode is also safe.

The one issue is that we need to make sure the buffer we're posting over to
another thread isn't destroyed in the meantime.  Simplest way to do that is to
copy it.

[mdumitrescu, 2014/08/15 18:20:15]

Done.

+    }
+
     public void setHttpMethod(String method) {
         validateNotStarted();
         if (!("PUT".equals(method) || "POST".equals(method))) {
             throw new IllegalArgumentException("Only PUT or POST are allowed.");
         }
         mMethod = method;
     }
@@ -155,15 +216,15 @@ public class UrlRequest {
             validateNotRecycled();
 
             mStarted = true;
 
             String method = mMethod;
             if (method == null &&
                     ((mUploadData != null && mUploadData.length > 0) ||
-                      mUploadChannel != null)) {
+                      mUploadChannel != null || mAppendChunkCondition != null)) {
                 // Default to POST if there is data to upload but no method was
                 // specified.
                 method = "POST";
             }
 
             if (method != null) {
                 nativeSetMethod(mUrlRequestAdapter, method);
@@ -186,14 +247,17 @@ public class UrlRequest {
 
             if (mUploadData != null && mUploadData.length > 0) {
                 nativeSetUploadData(mUrlRequestAdapter, mUploadContentType,
                                     mUploadData);
             } else if (mUploadChannel != null) {
                 nativeSetUploadChannel(mUrlRequestAdapter, mUploadContentType,
                                        mUploadContentLength);
+            } else if (mAppendChunkCondition != null) {
+                nativeEnableChunkedUpload(mUrlRequestAdapter,
+                                          mUploadContentType);
             }
 
             nativeStart(mUrlRequestAdapter);
           }
     }
 
     public void cancel() {
@@ -284,14 +348,22 @@ public class UrlRequest {
         validateHeadersAvailable();
         ResponseHeadersMap result = new ResponseHeadersMap();
         nativeGetAllHeaders(mUrlRequestAdapter, result);
         return result;
     }
 
     /**
+     * A callback invoked when appending a chunk to the request has completed.
+     */
+    @CalledByNative
+    protected void onAppendChunkCompleted() {
+        mAppendChunkCondition.open();
+    }
+
+    /**
      * A callback invoked when the first chunk of the response has arrived.
      */
     @CalledByNative
     protected void onResponseStarted() {
         mContentType = nativeGetContentType(mUrlRequestAdapter);
         mContentLength = nativeGetContentLength(mUrlRequestAdapter);
         mHeadersAvailable = true;
@@ -326,14 +398,17 @@ public class UrlRequest {
      * Notifies the listener, releases native data structures.
      */
     @SuppressWarnings("unused")
     @CalledByNative
     private void finish() {
         synchronized (mLock) {
             mFinished = true;
+            if (mAppendChunkCondition != null) {
+                mAppendChunkCondition.open();
+            }
 
             if (mRecycled) {
                 return;
             }
             try {
                 mSink.close();
             } catch (IOException e) {
@@ -421,14 +496,20 @@ public class UrlRequest {
 
     private native void nativeSetUploadData(long urlRequestAdapter,
             String contentType, byte[] content);
 
     private native void nativeSetUploadChannel(long urlRequestAdapter,
             String contentType, long contentLength);
 
+    private native void nativeEnableChunkedUpload(long urlRequestAdapter,
+            String contentType);
+
+    private native void nativeAppendChunk(long urlRequestAdapter,
+            ByteBuffer chunk, int chunkSize, boolean isLastChunk);
+
     private native void nativeStart(long urlRequestAdapter);
 
     private native void nativeCancel(long urlRequestAdapter);
 
     private native void nativeDestroyRequestAdapter(long urlRequestAdapter);
 
     private native int nativeGetErrorCode(long urlRequestAdapter);