[Cronet] Fix CronetFixedModeOutputStream to not write more bytes than specified

components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java

Index: components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java
diff --git a/components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java b/components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java
index f6e005335ae4104e6bf1b8444324ce89999908fb..d4fff6a32384da26c06cabdd4939976648438c27 100644
--- a/components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java
+++ b/components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java
@@ -60,13 +60,11 @@ final class CronetFixedModeOutputStream extends CronetOutputStream {
     @Override
     public void write(int oneByte) throws IOException {
         checkNotExceedContentLength(1);
-        while (mBuffer.position() == mBuffer.limit()) {
-            // Wait until buffer is consumed.
-            mMessageLoop.loop();
-        }
+        maybeWaitUntilBufferConsumed();
         mBuffer.put((byte) oneByte);
         mBytesWritten++;
         if (mBytesWritten == mContentLength) {
+            mBuffer.flip();
             // Entire post data has been received. Now wait for network stack to
             // read it.
             mMessageLoop.loop();
@@ -84,16 +82,14 @@ final class CronetFixedModeOutputStream extends CronetOutputStream {
         }

[pauljensen, 2016/06/10 15:05:25]

is there an important benfit to this early return here?  it looks like the rest
of the function should work fine with a |count| of 0 and execute quickly. 
Removing this if statement would make this function more readable.

[xunjieli, 2016/06/10 16:09:18]

Done.

         int toSend = count;
         while (toSend > 0) {
-            if (mBuffer.position() == mBuffer.limit()) {
-                // Wait until buffer is consumed.
-                mMessageLoop.loop();
-            }
-            int sent = Math.min(toSend, mBuffer.limit() - mBuffer.position());
+            maybeWaitUntilBufferConsumed();
+            int sent = Math.min(toSend, mBuffer.remaining());
             mBuffer.put(buffer, offset + count - toSend, sent);
             toSend -= sent;
         }
         mBytesWritten += count;
         if (mBytesWritten == mContentLength) {
+            mBuffer.flip();
             // Entire post data has been received. Now wait for network stack to
             // read it.
             mMessageLoop.loop();

[pauljensen, 2016/06/10 15:05:25]

can we combine these two lines from here and line line 67 and 104 into a
function called "uploadBufferInternal()"?

[xunjieli, 2016/06/10 16:09:18]

Done.

@@ -101,6 +97,18 @@ final class CronetFixedModeOutputStream extends CronetOutputStream {
     }
 
     /**
+     * If mBuffer is full, wait until {@code mBuffer} is consumed and there is
+     *  space to write more data to it.
+     */
+    private void maybeWaitUntilBufferConsumed() {

[pauljensen, 2016/06/10 15:05:25]

can we rename this to something a little easier to understand, like
ensureBufferNotFull() or ensureBufferHasRemaining()

[xunjieli, 2016/06/10 16:09:18]

Done.

+        while (mBuffer.position() == mBuffer.limit()) {
+            mBuffer.flip();

[pauljensen, 2016/06/10 15:05:25]

I don't think flip() should be in the loop as it has side effects that will
break the loop I think.  Perhaps we change this to:

if (!mBuffer.hasRemaining()) {
  mBuffer.flip();
  mMessageLoop.loop();
  assert mBuffer.remaining() == mBuffer.capacity();
}

[xunjieli, 2016/06/10 16:09:18]

Done.

+            // Wait until buffer is consumed.
+            mMessageLoop.loop();
+        }
+    }
+
+    /**
      * Throws {@link java.net.ProtocolException} if adding {@code numBytes} will
      * exceed content length.
      */
@@ -141,25 +149,20 @@ final class CronetFixedModeOutputStream extends CronetOutputStream {
 
         @Override
         public void read(final UploadDataSink uploadDataSink, final ByteBuffer byteBuffer) {
-            final int availableSpace = byteBuffer.remaining();
-            if (availableSpace < mBuffer.position()) {
-                // byteBuffer does not have enough capacity, so only put a portion
-                // of mBuffer in it.
-                byteBuffer.put(mBuffer.array(), 0, availableSpace);
-                mBuffer.position(availableSpace);
-                // Move remaining buffer to the head of the buffer for use in the
-                // next read call.
-                mBuffer.compact();
-            } else {
-                // byteBuffer has enough capacity to hold the content of mBuffer.
-                mBuffer.flip();
+            if (byteBuffer.remaining() >= mBuffer.remaining()) {
                 byteBuffer.put(mBuffer);
                 // Reuse this buffer.
                 mBuffer.clear();
+                uploadDataSink.onReadSucceeded(false);

[pauljensen, 2016/06/10 15:05:25]

any particular reason you moved this up?

[xunjieli, 2016/06/10 16:09:18]

I think it might be better to notify the native stack first before we quit the
message loop. This is how it was set up in CronetChunkedOutputStream.java. I am
not sure if the previous approach has any re-entrancy issue.

                 // Quit message loop so embedder can write more data.
                 mMessageLoop.quit();
+            } else {
+                int oldLimit = mBuffer.limit();
+                mBuffer.limit(mBuffer.position() + byteBuffer.remaining());
+                byteBuffer.put(mBuffer);
+                mBuffer.limit(oldLimit);

[pauljensen, 2016/06/10 15:05:25]

I was going to recommend changing this to the shorter:

  mBuffer.position(mBuffer.position() + byteBuffer.remaining());
  byteBuffer.put(mBuffer.array(), mBuffer.position() - byteBuffer.remaining(),
byteBuffer.remaining());

But that won't work with my idea to swap mBuffer with |buffer| in write() if
|buffer| isn't array-backed.  So I'll not mention it :)

[xunjieli, 2016/06/10 16:09:18]

Done.

[pauljensen, 2016/06/10 17:45:44]

Um I wasn't intending you to implement this.  It's okay, I may revert back to
the old way when I implement the idea to send directly from the client's buffer
(which we can't assume to be array-backed and hence cannot call array() on).

[xunjieli, 2016/06/10 18:48:11]

Ah, I see. I was confused by "it". Done. I have reverted to the original.

+                uploadDataSink.onReadSucceeded(false);
             }
-            uploadDataSink.onReadSucceeded(false);
         }
 
         @Override