Fix race in MediaDevicesPermissionChecker.

Fix race in MediaDevicesPermissionChecker.

The race affected only unit tests.

BUG=648183
Committed: https://crrev.com/098908e55009da8b1aea455d7dc6b5a0b45bc7d0
Cr-Commit-Position: refs/heads/master@{#431069}

[Guido Urdaneta, 2016-11-03 13:43:27]

Description was changed from

==========
Fix race in MediaDevicesPermissionChecker.

BUG=648183
==========

to

==========
Fix race in MediaDevicesPermissionChecker.

BUG=648183
==========

[Guido Urdaneta, 2016-11-03 13:43:27]

guidou@chromium.org changed reviewers:
+ maxmorin@chromium.org, xhwang@chromium.org

[Guido Urdaneta, 2016-11-03 13:43:56]

Description was changed from

==========
Fix race in MediaDevicesPermissionChecker.

BUG=648183
==========

to

==========
Fix race in MediaDevicesPermissionChecker.

The race affected only unit tests.

BUG=648183
==========

[Guido Urdaneta, 2016-11-03 13:45:58]

Hi, PTAL

[Guido Urdaneta, 2016-11-03 13:48:33]

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

[commit-bot: I haz the power, 2016-11-03 13:48:56]

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

[Max Morin, 2016-11-03 13:57:40]

lgtm if you consider my comment.

https://codereview.chromium.org/2474923002/diff/1/content/browser/media/media...
File content/browser/media/media_devices_permission_checker.h (right):

https://codereview.chromium.org/2474923002/diff/1/content/browser/media/media...
content/browser/media/media_devices_permission_checker.h:87:
DISALLOW_COPY_AND_ASSIGN(MediaDevicesPermissionChecker);
Wouldn't it be easier to just allow copies so you don't need to wrap it in a
unique_ptr?

[Guido Urdaneta, 2016-11-03 14:12:07]

On 2016/11/03 13:57:40, Max Morin Chromium wrote:
> lgtm if you consider my comment.
> 
>
https://codereview.chromium.org/2474923002/diff/1/content/browser/media/media...
> File content/browser/media/media_devices_permission_checker.h (right):
> 
>
https://codereview.chromium.org/2474923002/diff/1/content/browser/media/media...
> content/browser/media/media_devices_permission_checker.h:87:
> DISALLOW_COPY_AND_ASSIGN(MediaDevicesPermissionChecker);
> Wouldn't it be easier to just allow copies so you don't need to wrap it in a
> unique_ptr?

I considered that approach (and several others), but I didn't like it. In that
case it would be necessary to force the copy constructor to run on the UI thread
to avoid the race.
If we go that way, then we should probably also add assignment and
OverridePermissionsForTesting on the UI thread because, why not?
The unique_ptr approach avoids these issues, at a reasonable cost.
Also, in production, if you don't need to reset the checker, you can use it
without a unique_ptr. 
If the kUseFakeUIForMediaStream command line flag is enough for the overriding
needs of a test, the unique_ptr is also not necessary.

[Max Morin, 2016-11-03 14:25:09]

On 2016/11/03 14:12:07, Guido Urdaneta wrote:
> On 2016/11/03 13:57:40, Max Morin Chromium wrote:
> > lgtm if you consider my comment.
> > 
> >
>
https://codereview.chromium.org/2474923002/diff/1/content/browser/media/media...
> > File content/browser/media/media_devices_permission_checker.h (right):
> > 
> >
>
https://codereview.chromium.org/2474923002/diff/1/content/browser/media/media...
> > content/browser/media/media_devices_permission_checker.h:87:
> > DISALLOW_COPY_AND_ASSIGN(MediaDevicesPermissionChecker);
> > Wouldn't it be easier to just allow copies so you don't need to wrap it in a
> > unique_ptr?
> 
> I considered that approach (and several others), but I didn't like it. In that
> case it would be necessary to force the copy constructor to run on the UI
thread
> to avoid the race.
> If we go that way, then we should probably also add assignment and
> OverridePermissionsForTesting on the UI thread because, why not?
> The unique_ptr approach avoids these issues, at a reasonable cost.
> Also, in production, if you don't need to reset the checker, you can use it
> without a unique_ptr. 
> If the kUseFakeUIForMediaStream command line flag is enough for the overriding
> needs of a test, the unique_ptr is also not necessary.

Ok, thanks for fixing this.

[commit-bot: I haz the power, 2016-11-03 14:32:49]

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

[commit-bot: I haz the power, 2016-11-03 14:32:49]

Dry run: This issue passed the CQ dry run.

[xhwang, 2016-11-09 19:31:26]

media OWNERS rs lgtm

But I am not familiar with this code. Feel free to add proper OWNERS for
media_devices* files in the OWNERS files.

[Guido Urdaneta, 2016-11-09 20:26:41]

The CQ bit was checked by guidou@chromium.org

[Guido Urdaneta, 2016-11-09 20:26:42]

The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org,
maxmorin@chromium.org
Link to the patchset: https://codereview.chromium.org/2474923002/#ps20001
(title: "Add OWNERS file")

[commit-bot: I haz the power, 2016-11-09 20:27:13]

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, 2016-11-09 20:30:49]

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

[commit-bot: I haz the power, 2016-11-09 20:30:50]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[Guido Urdaneta, 2016-11-09 20:38:26]

The CQ bit was checked by guidou@chromium.org

[Guido Urdaneta, 2016-11-09 20:38:27]

The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org,
maxmorin@chromium.org
Link to the patchset: https://codereview.chromium.org/2474923002/#ps40001
(title: "rebase")

[commit-bot: I haz the power, 2016-11-09 20:38:48]

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, 2016-11-09 20:52:23]

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

[commit-bot: I haz the power, 2016-11-09 20:52:24]

Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Guido Urdaneta, 2016-11-09 22:10:39]

The CQ bit was checked by guidou@chromium.org

[Guido Urdaneta, 2016-11-09 22:10:40]

The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org,
maxmorin@chromium.org
Link to the patchset: https://codereview.chromium.org/2474923002/#ps60001
(title: "Fix rebase")

[commit-bot: I haz the power, 2016-11-09 22:11:18]

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, 2016-11-09 23:06:55]

Description was changed from

==========
Fix race in MediaDevicesPermissionChecker.

The race affected only unit tests.

BUG=648183
==========

to

==========
Fix race in MediaDevicesPermissionChecker.

The race affected only unit tests.

BUG=648183
==========

[commit-bot: I haz the power, 2016-11-09 23:06:57]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-11-09 23:15:53]

Description was changed from

==========
Fix race in MediaDevicesPermissionChecker.

The race affected only unit tests.

BUG=648183
==========

to

==========
Fix race in MediaDevicesPermissionChecker.

The race affected only unit tests.

BUG=648183

Committed: https://crrev.com/098908e55009da8b1aea455d7dc6b5a0b45bc7d0
Cr-Commit-Position: refs/heads/master@{#431069}
==========

[commit-bot: I haz the power, 2016-11-09 23:15:54]

Patchset 4 (id:??) landed as
https://crrev.com/098908e55009da8b1aea455d7dc6b5a0b45bc7d0
Cr-Commit-Position: refs/heads/master@{#431069}