Revert "[cronet] Re-use Direct ByteBuffer for Cronet upload"

This reverts commit 9580368.

Reason for revert: Suspect that there is a leak in this CL

Original change's description:
> [cronet] Re-use Direct ByteBuffer for Cronet upload
>
> This CL makes Cronet upload reuse a Java ByteBuffer object if the
> underlying net::IOBuffer's address and buffer length are unchanged.
>
> This should reduce the number of constructor calls to
> NewDirectByteBuffer().
>
> Bug: 756841
> Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester
> Change-Id: I751242095e2ba5793750d5a91e2bc3b10ec8b7a1
> Reviewed-on: https://chromium-review.googlesource.com/624196
> Reviewed-by: Andrei Kapishnikov <[email protected]>
> Commit-Queue: Helen Li <[email protected]>
> Cr-Commit-Position: refs/heads/master@{#496067}

[email protected], [email protected]


(cherry picked from commit f9121d1)

Bug: 756841
Change-Id: I186de0df7b316e4284648b1b9cca1addc7f7845d
Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester
Reviewed-on: https://chromium-review.googlesource.com/656109
Reviewed-by: Helen Li <[email protected]>
Commit-Queue: Helen Li <[email protected]>
Cr-Original-Commit-Position: refs/heads/master@{#500411}
Reviewed-on: https://chromium-review.googlesource.com/656002
Cr-Commit-Position: refs/branch-heads/3202@{#72}
Cr-Branched-From: fa6a5d8-refs/heads/master@{#499098}

components/cronet/android/cronet_upload_data_stream_adapter.cc

@@ -11,11 +11,9 @@
 #include "base/android/jni_string.h"
 #include "base/bind.h"
 #include "base/logging.h"
-#include "base/memory/ptr_util.h"
 #include "base/single_thread_task_runner.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "components/cronet/android/cronet_url_request_adapter.h"
-#include "components/cronet/android/io_buffer_with_byte_buffer.h"
 #include "jni/CronetUploadDataStream_jni.h"
 
 using base::android::JavaParamRef;
@@ -46,16 +44,15 @@ void CronetUploadDataStreamAdapter::Read(net::IOBuffer* buffer, int buf_len) {
   DCHECK(network_task_runner_);
   DCHECK(network_task_runner_->BelongsToCurrentThread());
   DCHECK_GT(buf_len, 0);
+  DCHECK(!buffer_.get());
+  buffer_ = buffer;
 
+  // TODO(mmenke):  Consider preserving the java buffer across reads, when the
+  // IOBuffer's data pointer and its length are unchanged.
   JNIEnv* env = base::android::AttachCurrentThread();
-  // Allow buffer reuse if |buffer| and |buf_len| are exactly the same as the
-  // ones used last time.
-  if (!(buffer_ && buffer_->io_buffer()->data() == buffer->data() &&
-        buffer_->io_buffer_len() == buf_len)) {
-    buffer_ = base::MakeUnique<ByteBufferWithIOBuffer>(env, buffer, buf_len);
-  }
-  Java_CronetUploadDataStream_readData(env, jupload_data_stream_,
-                                       buffer_->byte_buffer());
+  base::android::ScopedJavaLocalRef<jobject> java_buffer(
+      env, env->NewDirectByteBuffer(buffer->data(), buf_len));
+  Java_CronetUploadDataStream_readData(env, jupload_data_stream_, java_buffer);
 }
 
 void CronetUploadDataStreamAdapter::Rewind() {
@@ -85,6 +82,7 @@ void CronetUploadDataStreamAdapter::OnReadSucceeded(
     bool final_chunk) {
   DCHECK(bytes_read > 0 || (final_chunk && bytes_read == 0));
 
+  buffer_ = nullptr;
   network_task_runner_->PostTask(
       FROM_HERE, base::Bind(&CronetUploadDataStream::OnReadSuccess,
                             upload_data_stream_, bytes_read, final_chunk));

components/cronet/android/cronet_upload_data_stream_adapter.h

@@ -19,7 +19,6 @@ class SingleThreadTaskRunner;
 }  // namespace base
 
 namespace cronet {
-class ByteBufferWithIOBuffer;
 
 // The Adapter holds onto a reference to the IOBuffer that is currently being
 // written to in Java, so may not be deleted until any read operation in Java
@@ -67,8 +66,9 @@ class CronetUploadDataStreamAdapter : public CronetUploadDataStream::Delegate {
   scoped_refptr<base::SingleThreadTaskRunner> network_task_runner_;
   base::WeakPtr<CronetUploadDataStream> upload_data_stream_;
 
-  // Keeps the net::IOBuffer and Java ByteBuffer alive until the next Read().
-  std::unique_ptr<ByteBufferWithIOBuffer> buffer_;
+  // Used to keep the read buffer alive until the callback from Java has been
+  // received.
+  scoped_refptr<net::IOBuffer> buffer_;
 
   DISALLOW_COPY_AND_ASSIGN(CronetUploadDataStreamAdapter);
 };

components/cronet/android/io_buffer_with_byte_buffer.cc

@@ -24,14 +24,4 @@ IOBufferWithByteBuffer::IOBufferWithByteBuffer(
 
 IOBufferWithByteBuffer::~IOBufferWithByteBuffer() {}
 
-ByteBufferWithIOBuffer::ByteBufferWithIOBuffer(JNIEnv* env,
-                                               net::IOBuffer* io_buffer,
-                                               int io_buffer_len)
-    : io_buffer_(io_buffer), io_buffer_len_(io_buffer_len) {
-  byte_buffer_.Reset(
-      env, env->NewDirectByteBuffer(io_buffer_->data(), io_buffer_len_));
-}
-
-ByteBufferWithIOBuffer::~ByteBufferWithIOBuffer() {}
-
 }  // namespace cronet

components/cronet/android/io_buffer_with_byte_buffer.h

@@ -48,31 +48,6 @@ class IOBufferWithByteBuffer : public net::WrappedIOBuffer {
   DISALLOW_COPY_AND_ASSIGN(IOBufferWithByteBuffer);
 };
 
-// Represents a Java direct ByteBuffer backed by a net::IOBuffer. Keeps both the
-// net::IOBuffer and the Java ByteBuffer object alive until destroyed.
-class ByteBufferWithIOBuffer {
- public:
-  ByteBufferWithIOBuffer(JNIEnv* env,
-                         net::IOBuffer* io_buffer,
-                         int io_buffer_len);
-
-  ~ByteBufferWithIOBuffer();
-  const net::IOBuffer* io_buffer() const { return io_buffer_.get(); }
-  int io_buffer_len() const { return io_buffer_len_; }
-
-  const base::android::JavaRef<jobject>& byte_buffer() const {
-    return byte_buffer_;
-  }
-
- private:
-  scoped_refptr<net::IOBuffer> io_buffer_;
-  int io_buffer_len_;
-
-  base::android::ScopedJavaGlobalRef<jobject> byte_buffer_;
-
-  DISALLOW_COPY_AND_ASSIGN(ByteBufferWithIOBuffer);
-};
-
 }  // namespace cronet
 
 #endif  // COMPONENTS_CRONET_ANDROID_IO_BUFFER_WITH_BYTE_BUFFER_H_

components/cronet/android/java/src/org/chromium/net/impl/CronetUploadDataStream.java

@@ -216,7 +216,6 @@ public void onReadSucceeded(boolean lastChunk) {
                         String.format("Read upload data length %d exceeds expected length %d",
                                 mLength - mRemainingLength, mLength));
             }
-            mByteBuffer.position(0);
             mByteBuffer = null;
             mInWhichUserCallback = UserCallback.NOT_IN_CALLBACK;