Fix double close in MojoAudioOutputStream.

Fix double close in MojoAudioOutputStream.

Currently, the sync socket in MojoAudioOutputStream is closed by both
Mojo and the SyncSocket destructor. This causes problems if a handle
happens to be reused.

BUG=709394
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2809673002
Cr-Commit-Position: refs/heads/master@{#464374}
Committed: https://chromium.googlesource.com/chromium/src/+/d4bcb119316cfccf5c1ddcebe3b6d65a7fec09e4

[Max Morin, 2017-04-10 14:59:51]

Description was changed from

==========
Fix double close in MojoAudioOutputStream.

Currently, the sync socket in MojoAudioOutputStream is closed by both
Mojo and the SyncSocket destructor. This causes problems if a handle
happens to be reused.

BUG=709394
==========

to

==========
Fix double close in MojoAudioOutputStream.

Currently, the sync socket in MojoAudioOutputStream is closed by both
Mojo and the SyncSocket destructor. This causes problems if a handle
happens to be reused.

BUG=709394
==========

[Max Morin, 2017-04-10 14:59:51]

maxmorin@chromium.org changed reviewers:
+ dalecurtis@chromium.org, thakis@chromium.org

[Max Morin, 2017-04-10 15:00:16]

The CQ bit was checked by maxmorin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-10 15:00:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Max Morin, 2017-04-10 15:00:55]

Nico, Dale: PTAL.

[Nico, 2017-04-10 15:29:35]

Is it possible to write a media unit test for the double release?

base/ lgtm

https://codereview.chromium.org/2809673002/diff/20001/media/mojo/services/moj...
File media/mojo/services/mojo_audio_output_stream.cc (right):

https://codereview.chromium.org/2809673002/diff/20001/media/mojo/services/moj...
media/mojo/services/mojo_audio_output_stream.cc:74:
mojo::WrapPlatformFile(foreign_socket->Release());
Should WrapPltformFile take an rvalue of some kind to make this kind of mistake
impossible through static typing?

[Max Morin, 2017-04-10 15:44:20]

Thanks. I'll try to write a test for this tomorrow.

https://codereview.chromium.org/2809673002/diff/20001/media/mojo/services/moj...
File media/mojo/services/mojo_audio_output_stream.cc (right):

https://codereview.chromium.org/2809673002/diff/20001/media/mojo/services/moj...
media/mojo/services/mojo_audio_output_stream.cc:74:
mojo::WrapPlatformFile(foreign_socket->Release());
On 2017/04/10 15:29:35, Nico wrote:
> Should WrapPltformFile take an rvalue of some kind to make this kind of
mistake
> impossible through static typing?

It should probably take a ScopedPlatformHandle (and manually closed handles
shouldn't be used ever). I think this would be a lot of work to change though.

[Nico, 2017-04-10 15:50:37]

https://codereview.chromium.org/2809673002/diff/20001/media/mojo/services/moj...
File media/mojo/services/mojo_audio_output_stream.cc (right):

https://codereview.chromium.org/2809673002/diff/20001/media/mojo/services/moj...
media/mojo/services/mojo_audio_output_stream.cc:74:
mojo::WrapPlatformFile(foreign_socket->Release());
On 2017/04/10 15:44:20, Max Morin wrote:
> On 2017/04/10 15:29:35, Nico wrote:
> > Should WrapPltformFile take an rvalue of some kind to make this kind of
> mistake
> > impossible through static typing?
> 
> It should probably take a ScopedPlatformHandle (and manually closed handles
> shouldn't be used ever). I think this would be a lot of work to change though.

Maybe at least file a bug.

[DaleCurtis, 2017-04-10 19:24:48]

This is a bit weird, you're ripping the handle out of the SyncSocket when you
make this call, rendering it useless to the owner -- the fact that no current
owner does anything with is why it works. It seems like we would be passing
ownership of the foreign socket to the OnStreamCreated() method. WDYT?

[commit-bot: I haz the power, 2017-04-10 19:34:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-10 19:34:25]

Dry run: This issue passed the CQ dry run.

[Max Morin, 2017-04-11 08:53:35]

Description was changed from

==========
Fix double close in MojoAudioOutputStream.

Currently, the sync socket in MojoAudioOutputStream is closed by both
Mojo and the SyncSocket destructor. This causes problems if a handle
happens to be reused.

BUG=709394
==========

to

==========
Fix double close in MojoAudioOutputStream.

Currently, the sync socket in MojoAudioOutputStream is closed by both
Mojo and the SyncSocket destructor. This causes problems if a handle
happens to be reused.

BUG=709394
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[Max Morin, 2017-04-11 09:07:26]

On 2017/04/10 19:24:48, DaleCurtis wrote:
> This is a bit weird, you're ripping the handle out of the SyncSocket when you
> make this call, rendering it useless to the owner -- the fact that no current
> owner does anything with is why it works. It seems like we would be passing
> ownership of the foreign socket to the OnStreamCreated() method. WDYT?

This makes sense. I changed the interface to use unique_ptr to clarify the
ownership.

[Max Morin, 2017-04-11 09:11:11]

https://codereview.chromium.org/2809673002/diff/20001/media/mojo/services/moj...
File media/mojo/services/mojo_audio_output_stream.cc (right):

https://codereview.chromium.org/2809673002/diff/20001/media/mojo/services/moj...
media/mojo/services/mojo_audio_output_stream.cc:74:
mojo::WrapPlatformFile(foreign_socket->Release());
On 2017/04/10 15:50:37, Nico wrote:
> On 2017/04/10 15:44:20, Max Morin wrote:
> > On 2017/04/10 15:29:35, Nico wrote:
> > > Should WrapPltformFile take an rvalue of some kind to make this kind of
> > mistake
> > > impossible through static typing?
> > 
> > It should probably take a ScopedPlatformHandle (and manually closed handles
> > shouldn't be used ever). I think this would be a lot of work to change
though.
> 
> Maybe at least file a bug.

I filed crbug.com/710376 and also added checks for double-closing to the unit
tests.

[Max Morin, 2017-04-11 09:12:56]

The CQ bit was checked by maxmorin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-11 09:13:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-11 09:48:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-11 09:48:58]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Max Morin, 2017-04-11 10:16:28]

The CQ bit was checked by maxmorin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-11 10:16:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-11 11:42:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-11 11:42:38]

Dry run: This issue passed the CQ dry run.

[DaleCurtis, 2017-04-11 18:22:44]

https://codereview.chromium.org/2809673002/diff/80001/content/browser/rendere...
File content/browser/renderer_host/media/audio_renderer_host.cc (right):

https://codereview.chromium.org/2809673002/diff/80001/content/browser/rendere...
content/browser/renderer_host/media/audio_renderer_host.cc:110:
std::unique_ptr<base::CancelableSyncSocket> foreign_socket) {
You need something to own this here now or the socket is going to be closed no?

[Max Morin, 2017-04-12 08:16:06]

https://codereview.chromium.org/2809673002/diff/80001/content/browser/rendere...
File content/browser/renderer_host/media/audio_renderer_host.cc (right):

https://codereview.chromium.org/2809673002/diff/80001/content/browser/rendere...
content/browser/renderer_host/media/audio_renderer_host.cc:110:
std::unique_ptr<base::CancelableSyncSocket> foreign_socket) {
On 2017/04/11 18:22:44, DaleCurtis wrote:
> You need something to own this here now or the socket is going to be closed
no?

It's fine to close the socket handle on this end, as the socket is only used by
the remote end.

[DaleCurtis, 2017-04-12 18:28:54]

lgtm then, thanks!

[Max Morin, 2017-04-13 10:08:18]

Thanks for the review!

[Max Morin, 2017-04-13 10:08:25]

The CQ bit was checked by maxmorin@chromium.org

[Max Morin, 2017-04-13 10:08:26]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/2809673002/#ps80001
(title: "Fix Win complie.")

[commit-bot: I haz the power, 2017-04-13 10:08:46]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-13 11:43:17]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1492078105813330,
"parent_rev": "322a025409d5e5897720f0a818426a3e37cc4a96", "commit_rev":
"d4bcb119316cfccf5c1ddcebe3b6d65a7fec09e4"}

[commit-bot: I haz the power, 2017-04-13 11:44:06]

Description was changed from

==========
Fix double close in MojoAudioOutputStream.

Currently, the sync socket in MojoAudioOutputStream is closed by both
Mojo and the SyncSocket destructor. This causes problems if a handle
happens to be reused.

BUG=709394
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Fix double close in MojoAudioOutputStream.

Currently, the sync socket in MojoAudioOutputStream is closed by both
Mojo and the SyncSocket destructor. This causes problems if a handle
happens to be reused.

BUG=709394
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2809673002
Cr-Commit-Position: refs/heads/master@{#464374}
Committed:
https://chromium.googlesource.com/chromium/src/+/d4bcb119316cfccf5c1ddcebe3b6...
==========

[commit-bot: I haz the power, 2017-04-13 11:44:07]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/d4bcb119316cfccf5c1ddcebe3b6...