Do not pass the origin to MediaDevicesDispatcherHost.

Do not pass the origin to MediaDevicesDispatcherHost.

Instead get the origin from the corresponding RenderFrameHost.
Unique origins are now treated like any other insecure origin.

BUG=720025
Review-Url: https://codereview.chromium.org/2872913003
Cr-Commit-Position: refs/heads/master@{#471699}
Committed: https://chromium.googlesource.com/chromium/src/+/a72075abe9fe56104e33384f5876daa42a946a42

[Guido Urdaneta, 2017-05-09 17:55:37]

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

[Guido Urdaneta, 2017-05-09 17:55:54]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2017-05-09 17:56:56]

Dry run: CQ is trying da patch.

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

[Guido Urdaneta, 2017-05-09 17:57:03]

Description was changed from

==========
Do not pass the origin to MediaDevicesDispatcherHost.

Instead get the origin from the corresponding RenderFrameHost.

BUG=720025
==========

to

==========
Do not pass the origin to MediaDevicesDispatcherHost.

Instead get the origin from the corresponding RenderFrameHost.
The unique origin is now treated as any other insecure origin.

BUG=720025
==========

[Guido Urdaneta, 2017-05-09 17:57:26]

Description was changed from

==========
Do not pass the origin to MediaDevicesDispatcherHost.

Instead get the origin from the corresponding RenderFrameHost.
The unique origin is now treated as any other insecure origin.

BUG=720025
==========

to

==========
Do not pass the origin to MediaDevicesDispatcherHost.

Instead get the origin from the corresponding RenderFrameHost.
Unique origins are now treated like any other insecure origin.

BUG=720025
==========

[commit-bot: I haz the power, 2017-05-09 18:38:31]

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

[commit-bot: I haz the power, 2017-05-09 18:38:31]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[Guido Urdaneta, 2017-05-10 07:41:20]

guidou@chromium.org changed reviewers:
+ bbudge@chromium.org, hbos@chromium.org, mkwst@chromium.org

[Guido Urdaneta, 2017-05-10 07:41:20]

Hi, PTAL.

mkwst@chromium.org: Please review IPC and related changes in content/browser

hbos@chromium.org: Please review content/renderer/media

bbudge@chromium.org: Please review changes in content/renderer/pepper

[Mike West, 2017-05-10 08:52:22]

Thanks!

https://codereview.chromium.org/2872913003/diff/20001/content/browser/media/m...
File content/browser/media/media_devices_permission_checker.cc (right):

https://codereview.chromium.org/2872913003/diff/20001/content/browser/media/m...
content/browser/media/media_devices_permission_checker.cc:39: GURL origin =
frame_host->GetLastCommittedOrigin().GetURL();
I know this isn't part of the patch, but why is |origin| a `GURL` and not a
`url::Origin`?

https://codereview.chromium.org/2872913003/diff/20001/content/browser/rendere...
File content/browser/renderer_host/media/media_devices_dispatcher_host.cc
(right):

https://codereview.chromium.org/2872913003/diff/20001/content/browser/rendere...
content/browser/renderer_host/media/media_devices_dispatcher_host.cc:48: if
(!origin_for_testing.unique())
How do you intend to test the behavior for unique origins?

Perhaps `security_origin_for_testing_` could be a `std::unique_ptr<url::Origin>`
instead, and you could check against `nullptr` instead?

[Guido Urdaneta, 2017-05-10 09:38:01]

https://codereview.chromium.org/2872913003/diff/20001/content/browser/media/m...
File content/browser/media/media_devices_permission_checker.cc (right):

https://codereview.chromium.org/2872913003/diff/20001/content/browser/media/m...
content/browser/media/media_devices_permission_checker.cc:39: GURL origin =
frame_host->GetLastCommittedOrigin().GetURL();
On 2017/05/10 08:52:22, Mike West wrote:
> I know this isn't part of the patch, but why is |origin| a `GURL` and not a
> `url::Origin`?

My guess is that it is or depends on old code that predates the introduction of
url::Origin.
I can file a bug to migrate to url::Origin in the lower layers (which are quite
a few, a quick search yields 63 versions of CheckMediaAccessPermission probably
all using GURL instead of url::Origin).

https://codereview.chromium.org/2872913003/diff/20001/content/browser/rendere...
File content/browser/renderer_host/media/media_devices_dispatcher_host.cc
(right):

https://codereview.chromium.org/2872913003/diff/20001/content/browser/rendere...
content/browser/renderer_host/media/media_devices_dispatcher_host.cc:48: if
(!origin_for_testing.unique())
On 2017/05/10 08:52:22, Mike West wrote:
> How do you intend to test the behavior for unique origins?
> 
> Perhaps `security_origin_for_testing_` could be a
`std::unique_ptr<url::Origin>`
> instead, and you could check against `nullptr` instead?

Is a base::Optional acceptable too?
Note that I removed special tests for unique origins since now they are treated
like any other unauthorized and insecure origin, but I can restore them.
Now that the origin comes from a more reliable source, I see no reason to treat
unique origins differently anymore.

[Guido Urdaneta, 2017-05-10 11:09:41]

https://codereview.chromium.org/2872913003/diff/20001/content/browser/rendere...
File content/browser/renderer_host/media/media_devices_dispatcher_host.cc
(right):

https://codereview.chromium.org/2872913003/diff/20001/content/browser/rendere...
content/browser/renderer_host/media/media_devices_dispatcher_host.cc:48: if
(!origin_for_testing.unique())
Note also that in unit tests, |frame_host| is null, so that's a way to test
unique origins.

[Mike West, 2017-05-11 07:46:11]

On 2017/05/10 at 09:38:01, guidou wrote:
> Note that I removed special tests for unique origins since now they are
treated like any other unauthorized and insecure origin, but I can restore them.
> Now that the origin comes from a more reliable source, I see no reason to
treat unique origins differently anymore.

As long as there's a test in place somewhere that proves that you're doing the
right thing in the presence of a sandboxed frame (e.g. one with an origin
indistinguishable from `new url::Origin()`), I'm happy. :)

This change LGTM iff such a test is present.

[Guido Urdaneta, 2017-05-11 09:53:17]

guidou@chromium.org changed reviewers:
+ jochen@chromium.org

[Guido Urdaneta, 2017-05-11 09:53:18]

jochen@chromium.org: Can you RS?

[Guido Urdaneta, 2017-05-12 09:52:41]

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

[commit-bot: I haz the power, 2017-05-12 09:53:02]

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-05-12 10:39:52]

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

[commit-bot: I haz the power, 2017-05-12 10:39:54]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Guido Urdaneta, 2017-05-12 14:59:16]

Friendly ping :)

[hbos_chromium, 2017-05-12 15:11:33]

content/renderer/media stampy LGTM, relying on other reviewers for implications
of removing security_origin

[jochen (gone - plz use gerrit), 2017-05-15 05:35:39]

lgtm

[Guido Urdaneta, 2017-05-15 07:42:19]

The CQ bit was checked by guidou@chromium.org

[Guido Urdaneta, 2017-05-15 07:42:19]

The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/2872913003/#ps40001
(title: "Add tests with unique origin")

[commit-bot: I haz the power, 2017-05-15 07:42: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-05-15 08:36:21]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1494834139178670,
"parent_rev": "776bdfd34d33dd06e8c9011a753c06a98cc87b9b", "commit_rev":
"a72075abe9fe56104e33384f5876daa42a946a42"}

[commit-bot: I haz the power, 2017-05-15 08:36:33]

Description was changed from

==========
Do not pass the origin to MediaDevicesDispatcherHost.

Instead get the origin from the corresponding RenderFrameHost.
Unique origins are now treated like any other insecure origin.

BUG=720025
==========

to

==========
Do not pass the origin to MediaDevicesDispatcherHost.

Instead get the origin from the corresponding RenderFrameHost.
Unique origins are now treated like any other insecure origin.

BUG=720025

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

[commit-bot: I haz the power, 2017-05-15 08:36:34]

Committed patchset #2 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/a72075abe9fe56104e33384f5876...