Close bitstream buffer shared memory handles on Android.

Close bitstream buffer shared memory handles on Android.

AndroidVideoDecodeAccelerator would clear references to pending
bitstream buffers in ::Reset and, implicitly, in the destructor.
However, each of those may have a file descriptor for shared memory
that isn't closed.  During a seek, for example, this can cause leaks.

This CL wraps the queue of pending bitstream buffers with a new
BitstreamRecord type, which has a unique_ptr to the (unmapped)
shared memory region, if any.  This will automatically close the
underlying handle when the unique_ptr is released.  Note that no
expensive operation is performed in creating the shared memory
region.  Unless it is later mapped, it just keeps the handle
and ::close()s it on destruction.

BUG=614489
TEST=observed /proc/fd/[gpu process] while seeking in a video

Committed: https://crrev.com/6e41c76f8bb74142e5a484f3ee6f6a87119b12fb
Cr-Commit-Position: refs/heads/master@{#395985}

[liberato (no reviews please), 2016-05-24 21:39:42]

Description was changed from

==========
Close bitstream buffer shared memory handles on Android.

AndroidVideoDecodeAccelerator would clear references to pending
bitstream buffers in ::Reset and, implicitly, in the destructor.
However, each of those may have a file descriptor for shared memory
that isn't closed.  During a seek, for example, this can cause leaks.

No examples of leaked fds was observed in the destructor, since no
pending bitstream buffers were present.

This CL modifies ::Reset and ::ActualDestroy to close any file
descriptors before clearing.  Note that ::ActualDestroy is the only
path to destroying the VDA.  Even though no case was observed, it
seems like a good idea to check in the destructor as well.

BUG=614489
TEST=observed /proc/fd/[gpu process] while seeking in a video
==========

to

==========
Close bitstream buffer shared memory handles on Android.

AndroidVideoDecodeAccelerator would clear references to pending
bitstream buffers in ::Reset and, implicitly, in the destructor.
However, each of those may have a file descriptor for shared memory
that isn't closed.  During a seek, for example, this can cause leaks.

No examples of leaked fds was observed in the destructor, since no
pending bitstream buffers were present.

This CL modifies ::Reset and ::ActualDestroy to close any file
descriptors before clearing.  Note that ::ActualDestroy is the only
path to destroying the VDA.  Even though no case was observed, it
seems like a good idea to check in the destructor as well.

BUG=614489
TEST=observed /proc/fd/[gpu process] while seeking in a video
==========

[liberato (no reviews please), 2016-05-24 21:39:42]

liberato@chromium.org changed reviewers:
+ dalecurtis@chromium.org, watk@chromium.org

[liberato (no reviews please), 2016-05-24 21:40:50]

yay for leaks.  on my n7 / lollipop, it's been seeking 3x videos for several
minutes with fds bounded by +/- 5 or so.

thanks
-fl

[watk, 2016-05-24 22:16:07]

https://codereview.chromium.org/2011653002/diff/1/media/gpu/android_video_dec...
File media/gpu/android_video_decode_accelerator.cc (right):

https://codereview.chromium.org/2011653002/diff/1/media/gpu/android_video_dec...
media/gpu/android_video_decode_accelerator.cc:1524: new
SharedMemoryRegion(bitstream_buffer, true));
We were thinking it might be safer to wrap the handle as soon as we enqueue it
in Decode so we can't make this mistake again. WDYT?

[liberato (no reviews please), 2016-05-24 23:19:26]

https://codereview.chromium.org/2011653002/diff/1/media/gpu/android_video_dec...
File media/gpu/android_video_decode_accelerator.cc (right):

https://codereview.chromium.org/2011653002/diff/1/media/gpu/android_video_dec...
media/gpu/android_video_decode_accelerator.cc:1524: new
SharedMemoryRegion(bitstream_buffer, true));
On 2016/05/24 22:16:07, watk wrote:
> We were thinking it might be safer to wrap the handle as soon as we enqueue it
> in Decode so we can't make this mistake again. WDYT?

SMR is non-copyable, so i hadn't considered it.  but looking back, that makes
little sense.  we're handing around ptrs.

i'll do that instead.  thanks!

[liberato (no reviews please), 2016-05-25 13:47:48]

this has watk's RAII suggestion.  i've tested it via chromoting, and it appears
like it works as intended.  i'll do more testing when i'm back in the office.

-fl

[DaleCurtis, 2016-05-25 19:16:30]

lgtm

[watk, 2016-05-25 20:07:44]

Nice, the description is a bit out of date now btw. lgtm

[liberato (no reviews please), 2016-05-25 20:39:25]

Description was changed from

==========
Close bitstream buffer shared memory handles on Android.

AndroidVideoDecodeAccelerator would clear references to pending
bitstream buffers in ::Reset and, implicitly, in the destructor.
However, each of those may have a file descriptor for shared memory
that isn't closed.  During a seek, for example, this can cause leaks.

No examples of leaked fds was observed in the destructor, since no
pending bitstream buffers were present.

This CL modifies ::Reset and ::ActualDestroy to close any file
descriptors before clearing.  Note that ::ActualDestroy is the only
path to destroying the VDA.  Even though no case was observed, it
seems like a good idea to check in the destructor as well.

BUG=614489
TEST=observed /proc/fd/[gpu process] while seeking in a video
==========

to

==========
Close bitstream buffer shared memory handles on Android.

AndroidVideoDecodeAccelerator would clear references to pending
bitstream buffers in ::Reset and, implicitly, in the destructor.
However, each of those may have a file descriptor for shared memory
that isn't closed.  During a seek, for example, this can cause leaks.

This CL wraps the queue of pending bitstream buffers with a new
BitstreamRecord type, which has a unique_ptr to the (unmapped)
shared memory region, if any.  This will automatically close the
underlying handle when the unique_ptr is released.  Note that no
expensive operation is performed in creating the shared memory
region.  Unless it is later mapped, it just keeps the handle
and ::close()s it on destruction.

BUG=614489
TEST=observed /proc/fd/[gpu process] while seeking in a video
==========

[liberato (no reviews please), 2016-05-25 20:47:46]

On 2016/05/25 20:07:44, watk wrote:
> Nice, the description is a bit out of date now btw. lgtm

thanks -- i always forget the git log means nothing after the inital upload.

[liberato (no reviews please), 2016-05-25 20:47:49]

The CQ bit was checked by liberato@chromium.org

[commit-bot: I haz the power, 2016-05-25 20:48:11]

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

[commit-bot: I haz the power, 2016-05-25 20:52:35]

Committed patchset #3 (id:20001)

[commit-bot: I haz the power, 2016-05-25 20:54:02]

Description was changed from

==========
Close bitstream buffer shared memory handles on Android.

AndroidVideoDecodeAccelerator would clear references to pending
bitstream buffers in ::Reset and, implicitly, in the destructor.
However, each of those may have a file descriptor for shared memory
that isn't closed.  During a seek, for example, this can cause leaks.

This CL wraps the queue of pending bitstream buffers with a new
BitstreamRecord type, which has a unique_ptr to the (unmapped)
shared memory region, if any.  This will automatically close the
underlying handle when the unique_ptr is released.  Note that no
expensive operation is performed in creating the shared memory
region.  Unless it is later mapped, it just keeps the handle
and ::close()s it on destruction.

BUG=614489
TEST=observed /proc/fd/[gpu process] while seeking in a video
==========

to

==========
Close bitstream buffer shared memory handles on Android.

AndroidVideoDecodeAccelerator would clear references to pending
bitstream buffers in ::Reset and, implicitly, in the destructor.
However, each of those may have a file descriptor for shared memory
that isn't closed.  During a seek, for example, this can cause leaks.

This CL wraps the queue of pending bitstream buffers with a new
BitstreamRecord type, which has a unique_ptr to the (unmapped)
shared memory region, if any.  This will automatically close the
underlying handle when the unique_ptr is released.  Note that no
expensive operation is performed in creating the shared memory
region.  Unless it is later mapped, it just keeps the handle
and ::close()s it on destruction.

BUG=614489
TEST=observed /proc/fd/[gpu process] while seeking in a video

Committed: https://crrev.com/6e41c76f8bb74142e5a484f3ee6f6a87119b12fb
Cr-Commit-Position: refs/heads/master@{#395985}
==========

[commit-bot: I haz the power, 2016-05-25 20:54:03]

Patchset 3 (id:??) landed as
https://crrev.com/6e41c76f8bb74142e5a484f3ee6f6a87119b12fb
Cr-Commit-Position: refs/heads/master@{#395985}