Fix leak in CronetBidirectionalStreamAdapter::OnDataSent

components/cronet/android/cronet_bidirectional_stream_adapter.cc

 // Copyright 2015 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 "cronet_bidirectional_stream_adapter.h"
 
 #include <string>
 #include <vector>
 
 #include "base/bind.h"
@@ skipping 240 matching lines @@
   // embedder releases it, too.
   read_buffer_ = nullptr;
 }
 
 void CronetBidirectionalStreamAdapter::OnDataSent() {
   DCHECK(context_->IsOnNetworkThread());
   DCHECK(!write_buffer_list_.empty());
 
   JNIEnv* env = base::android::AttachCurrentThread();
   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.
   write_buffer_list_.clear();
 }
 
 void CronetBidirectionalStreamAdapter::OnTrailersReceived(
     const net::SpdyHeaderBlock& response_trailers) {
   DCHECK(context_->IsOnNetworkThread());
   JNIEnv* env = base::android::AttachCurrentThread();
@@ skipping 97 matching lines @@
 
   std::vector<std::string> headers;
   for (const auto& header : header_block) {
     headers.push_back(header.first.as_string());
     headers.push_back(header.second.as_string());
   }
   return base::android::ToJavaArrayOfStrings(env, headers);
 }
 
 }  // namespace cronet