Fix leak in CronetBidirectionalStreamAdapter::OnDataSent

components/cronet/android/cronet_bidirectional_stream_adapter.cc

Index: components/cronet/android/cronet_bidirectional_stream_adapter.cc
diff --git a/components/cronet/android/cronet_bidirectional_stream_adapter.cc b/components/cronet/android/cronet_bidirectional_stream_adapter.cc
index 84ab85dacb426a7d514dcbe09d6d30108a8ce8e9..64ed654b26ab0c1a181b6725a60776e4bbf4f56d 100644
--- a/components/cronet/android/cronet_bidirectional_stream_adapter.cc
+++ b/components/cronet/android/cronet_bidirectional_stream_adapter.cc
@@ -260,24 +260,25 @@ void CronetBidirectionalStreamAdapter::OnDataSent() {
   base::android::ScopedJavaLocalRef<jclass> byte_buffer_clazz(
       env, env->GetObjectClass(write_buffer_list_[0]->byte_buffer()));

[mef, 2016/05/09 18:35:13]

This seems pretty inefficient, and ALSO wrong. You are getting the class of
first ByteBuffer that was passed to WriteV, but there should be no need for that
as it should always be the ByteBuffer class.

In fact, can we just keep the array passed down to WriteV and pass it back to
onWriteCompleted? That would avoid the allocation, referencing, etc.

[xunjieli, 2016/05/09 21:31:14]

Done.
Can we handle this in a follow-up? We currently saves the list of
IOBufferWithByteBuffer.

[mef, 2016/05/09 21:48:15]

sg to limit this CL to fix of the leak.

   size_t size = write_buffer_list_.size();
-  jobjectArray jbuffers =
+  jobjectArray jbuffer_array =
       env->NewObjectArray(size, byte_buffer_clazz.obj(), NULL);
   base::android::CheckException(env);
   std::vector<int> initial_positions;
   std::vector<int> initial_limits;
   for (size_t i = 0; i < size; ++i) {
-    env->SetObjectArrayElement(jbuffers, i,
+    env->SetObjectArrayElement(jbuffer_array, i,
                                write_buffer_list_[i]->byte_buffer());

[mef, 2016/05/09 18:35:12]

nit: It took me a little bit to understand that IOByteBufferList is actually
list of IOBufferWithByteBuffer objects. Maybe rename IOByteBufferList ->
IOBufferWithByteBufferList?

[xunjieli, 2016/05/09 21:31:14]

Done.

     initial_positions.push_back(write_buffer_list_[i]->initial_position());
     initial_limits.push_back(write_buffer_list_[i]->initial_limit());
   }
+  ScopedJavaLocalRef<jobjectArray> jbuffers(env, jbuffer_array);

[mef, 2016/05/09 18:35:12]

Could this be combined with allocation of NewObjectArray on line 263?

[xunjieli, 2016/05/09 21:31:15]

Not that I know of. I am following the example in jni_array.cc

   ScopedJavaLocalRef<jintArray> jinitial_positions =
       base::android::ToJavaIntArray(env, initial_positions);
   ScopedJavaLocalRef<jintArray> jinitial_limits =
       base::android::ToJavaIntArray(env, initial_limits);
   // Call into Java.
   cronet::Java_CronetBidirectionalStream_onWritevCompleted(
-      env, owner_.obj(), jbuffers, jinitial_positions.obj(),
+      env, owner_.obj(), jbuffers.obj(), jinitial_positions.obj(),
       jinitial_limits.obj(), write_end_of_stream_);
   // Free the write buffers. This lets the Java ByteBuffer be freed, if the
   // embedder releases it, too.