Fix leak in CronetBidirectionalStreamAdapter::OnDataSent

Fix leak in CronetBidirectionalStreamAdapter::OnDataSent

This CL wraps the newly created jobjectArray with ScopedJavaLocalRef so that
the object is deleted when call returns.

BUG=610353
Committed: https://crrev.com/c524bde480633344575ff0a2b0b8a7489ee07156
Cr-Commit-Position: refs/heads/master@{#392463}

[xunjieli, 2016-05-09 17:27:28]

xunjieli@chromium.org changed reviewers:
+ mef@chromium.org

[xunjieli, 2016-05-09 17:27:28]

Misha: PTAL. Thanks!

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

https://codereview.chromium.org/1960243002/diff/1/components/cronet/android/c...
File components/cronet/android/cronet_bidirectional_stream_adapter.cc (right):

https://codereview.chromium.org/1960243002/diff/1/components/cronet/android/c...
components/cronet/android/cronet_bidirectional_stream_adapter.cc:261: env,
env->GetObjectClass(write_buffer_list_[0]->byte_buffer()));
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.

https://codereview.chromium.org/1960243002/diff/1/components/cronet/android/c...
components/cronet/android/cronet_bidirectional_stream_adapter.cc:270:
write_buffer_list_[i]->byte_buffer());
nit: It took me a little bit to understand that IOByteBufferList is actually
list of IOBufferWithByteBuffer objects. Maybe rename IOByteBufferList ->
IOBufferWithByteBufferList?

https://codereview.chromium.org/1960243002/diff/1/components/cronet/android/c...
components/cronet/android/cronet_bidirectional_stream_adapter.cc:274:
ScopedJavaLocalRef<jobjectArray> jbuffers(env, jbuffer_array);
Could this be combined with allocation of NewObjectArray on line 263?

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

Thanks!

https://codereview.chromium.org/1960243002/diff/1/components/cronet/android/c...
File components/cronet/android/cronet_bidirectional_stream_adapter.cc (right):

https://codereview.chromium.org/1960243002/diff/1/components/cronet/android/c...
components/cronet/android/cronet_bidirectional_stream_adapter.cc:261: env,
env->GetObjectClass(write_buffer_list_[0]->byte_buffer()));
On 2016/05/09 18:35:13, mef wrote:
> 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.

Done.
> 
> 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.

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

https://codereview.chromium.org/1960243002/diff/1/components/cronet/android/c...
components/cronet/android/cronet_bidirectional_stream_adapter.cc:270:
write_buffer_list_[i]->byte_buffer());
On 2016/05/09 18:35:12, mef wrote:
> nit: It took me a little bit to understand that IOByteBufferList is actually
> list of IOBufferWithByteBuffer objects. Maybe rename IOByteBufferList ->
> IOBufferWithByteBufferList?

Done.

https://codereview.chromium.org/1960243002/diff/1/components/cronet/android/c...
components/cronet/android/cronet_bidirectional_stream_adapter.cc:274:
ScopedJavaLocalRef<jobjectArray> jbuffers(env, jbuffer_array);
On 2016/05/09 18:35:12, mef wrote:
> Could this be combined with allocation of NewObjectArray on line 263?

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

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

lgtm

https://codereview.chromium.org/1960243002/diff/1/components/cronet/android/c...
File components/cronet/android/cronet_bidirectional_stream_adapter.cc (right):

https://codereview.chromium.org/1960243002/diff/1/components/cronet/android/c...
components/cronet/android/cronet_bidirectional_stream_adapter.cc:261: env,
env->GetObjectClass(write_buffer_list_[0]->byte_buffer()));
On 2016/05/09 21:31:14, xunjieli wrote:
> On 2016/05/09 18:35:13, mef wrote:
> > 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.
> 
> Done.
> > 
> > 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.
> 
> Can we handle this in a follow-up? We currently saves the list of
> IOBufferWithByteBuffer.

sg to limit this CL to fix of the leak.

[xunjieli, 2016-05-09 22:25:54]

The CQ bit was checked by xunjieli@chromium.org

[commit-bot: I haz the power, 2016-05-09 22:26:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1960243002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1960243002/20001

[commit-bot: I haz the power, 2016-05-09 23:05:41]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-05-09 23:06:50]

Description was changed from

==========
Fix leak in CronetBidirectionalStreamAdapter::OnDataSent

This CL wraps the newly created jobjectArray with ScopedJavaLocalRef so that
the object is deleted when call returns.

BUG=610353
==========

to

==========
Fix leak in CronetBidirectionalStreamAdapter::OnDataSent

This CL wraps the newly created jobjectArray with ScopedJavaLocalRef so that
the object is deleted when call returns.

BUG=610353

Committed: https://crrev.com/c524bde480633344575ff0a2b0b8a7489ee07156
Cr-Commit-Position: refs/heads/master@{#392463}
==========

[commit-bot: I haz the power, 2016-05-09 23:06:51]

Patchset 2 (id:??) landed as
https://crrev.com/c524bde480633344575ff0a2b0b8a7489ee07156
Cr-Commit-Position: refs/heads/master@{#392463}