Add metrics for usage of permission features from iframes in blink

Add metrics for usage of permission features from cross-origin iframes in blink

This CL adds metrics to measure the usage of features that require permissions
in Chrome when they are used in cross-origin iframes only. In particular we only
want to count an iframe if it doesn't have scripting access into the top level
document. A new UseCounter::countCrossOriginIframe function has been added which
will only count this type of usage.

Note that this intends to count cases when a site tries to use an API whether or
not the permission is granted by the user.

BUG=444744
Committed: https://crrev.com/fe356a2d22a8bf2590ce54d2c9c35cbc8ad0f819
Cr-Commit-Position: refs/heads/master@{#371756}

[raymes, 2016-01-13 06:20:54]

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

[commit-bot: I haz the power, 2016-01-13 06:21:00]

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

[commit-bot: I haz the power, 2016-01-13 07:25:46]

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

[commit-bot: I haz the power, 2016-01-13 07:25:46]

Dry run: This issue passed the CQ dry run.

[raymes, 2016-01-13 23:01:40]

Description was changed from

==========
Add metrics for usage of permission features from iframes in blink

BUG=444744
==========

to

==========
Add metrics for usage of permission features from iframes in blink

This CL adds metrics to measure the usage of features that require permissions
in Chrome when they are used in iframes only. A new UseCounter::countIframe
function has been added which will only count usage if it is not in the main
frame.

BUG=444744
==========

[raymes, 2016-01-13 23:02:28]

raymes@chromium.org changed reviewers:
+ mlamouri@chromium.org, timloh@chromium.org

[raymes, 2016-01-13 23:02:28]

Hey guys,

PTAL. I'd like to try to land this before branch point to get stats if possible.

Thanks!
Raymes

[raymes, 2016-01-13 23:05:36]

Description was changed from

==========
Add metrics for usage of permission features from iframes in blink

This CL adds metrics to measure the usage of features that require permissions
in Chrome when they are used in iframes only. A new UseCounter::countIframe
function has been added which will only count usage if it is not in the main
frame.

BUG=444744
==========

to

==========
Add metrics for usage of permission features from iframes in blink

This CL adds metrics to measure the usage of features that require permissions
in Chrome when they are used in iframes only. A new UseCounter::countIframe
function has been added which will only count usage if it is not in the main
frame.

BUG=444744
==========

[Timothy Loh, 2016-01-14 02:05:42]

On 2016/01/13 23:02:28, raymes wrote:
> Hey guys,
> 
> PTAL. I'd like to try to land this before branch point to get stats if
possible.
> 
> Thanks!
> Raymes

lgtm

[mlamouri (slow - plz ping), 2016-01-14 14:37:48]

Didn't we want to count cross origin iframes?

[raymes, 2016-01-18 04:19:23]

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

[commit-bot: I haz the power, 2016-01-18 04:19:27]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1578263006/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1578263006/140001

[raymes, 2016-01-18 04:21:06]

On 2016/01/14 14:37:48, Mounir Lamouri wrote:
> Didn't we want to count cross origin iframes?

Good catch, thanks Mounir! PTAL.

Also note that I tweaked the Notifications metric. It now measures all usage
either by creating a notification or even by just requesting the permission
(this is consistent with the other metrics).

[commit-bot: I haz the power, 2016-01-18 05:38:26]

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

[commit-bot: I haz the power, 2016-01-18 05:38:27]

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

[raymes, 2016-01-18 05:55:12]

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

[commit-bot: I haz the power, 2016-01-18 05:55:32]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1578263006/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1578263006/160001

[commit-bot: I haz the power, 2016-01-18 07:16:48]

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

[commit-bot: I haz the power, 2016-01-18 07:16:48]

Dry run: This issue passed the CQ dry run.

[raymes, 2016-01-19 06:07:13]

Patchset #11 (id:200001) has been deleted

[raymes, 2016-01-19 06:07:17]

Patchset #10 (id:180001) has been deleted

[mlamouri (slow - plz ping), 2016-01-19 17:54:49]

Looks good. I still have a few comments below. Could you also update the
description to match the current state of the patch?

https://codereview.chromium.org/1578263006/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/1578263006/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Element.cpp:3044:
UseCounter::countCrossOriginIframe(document(),
UseCounter::ElementRequestPointerLockIframe);
Maybe you could move this into `PointerLockController::requestPointerLock`?

https://codereview.chromium.org/1578263006/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/UseCounter.cpp (right):

https://codereview.chromium.org/1578263006/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/UseCounter.cpp:740: if (parent &&
!securityOrigin->canAccess(parent->securityContext()->securityOrigin()))
canAccess() is not exactly "cross origin". Maybe this should be specified in the
header.

https://codereview.chromium.org/1578263006/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/mediastream/UserMediaRequest.cpp (right):

https://codereview.chromium.org/1578263006/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediastream/UserMediaRequest.cpp:128:
UseCounter::countCrossOriginIframe(*document,
UseCounter::GetUserMediaSecureOriginIframe);
Should you add a GetUserMediaInsecureOriginIframe like you did for Geolocation?

https://codereview.chromium.org/1578263006/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/notifications/Notification.cpp (right):

https://codereview.chromium.org/1578263006/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/notifications/Notification.cpp:328: // Measure
usage in cross-origin iframes.
Why do we need to know that? If we want to know usage, requestPermission() is
going to add noise.

[raymes, 2016-01-20 00:03:19]

Description was changed from

==========
Add metrics for usage of permission features from iframes in blink

This CL adds metrics to measure the usage of features that require permissions
in Chrome when they are used in iframes only. A new UseCounter::countIframe
function has been added which will only count usage if it is not in the main
frame.

BUG=444744
==========

to

==========
Add metrics for usage of permission features from cross-origin iframes in blink

This CL adds metrics to measure the usage of features that require permissions
in Chrome when they are used in cross-origin iframes only. In particular we only
want to count an iframe if it doesn't have scripting access into the top level
document. A new UseCounter::countCrossOriginIframe
function has been added which will only count this type of usage.

Note that this intends to count cases when a site tries to use an API whether or
not the permission is granted by the user.

BUG=444744
==========

[raymes, 2016-01-20 00:04:37]

Description was changed from

==========
Add metrics for usage of permission features from cross-origin iframes in blink

This CL adds metrics to measure the usage of features that require permissions
in Chrome when they are used in cross-origin iframes only. In particular we only
want to count an iframe if it doesn't have scripting access into the top level
document. A new UseCounter::countCrossOriginIframe
function has been added which will only count this type of usage.

Note that this intends to count cases when a site tries to use an API whether or
not the permission is granted by the user.

BUG=444744
==========

to

==========
Add metrics for usage of permission features from cross-origin iframes in blink

This CL adds metrics to measure the usage of features that require permissions
in Chrome when they are used in cross-origin iframes only. In particular we only
want to count an iframe if it doesn't have scripting access into the top level
document. A new UseCounter::countCrossOriginIframe function has been added which
will only count this type of usage.

Note that this intends to count cases when a site tries to use an API whether or
not the permission is granted by the user.

BUG=444744
==========

[raymes, 2016-01-20 00:13:12]

Thanks Mounir

https://codereview.chromium.org/1578263006/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/1578263006/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Element.cpp:3044:
UseCounter::countCrossOriginIframe(document(),
UseCounter::ElementRequestPointerLockIframe);
On 2016/01/19 17:54:48, Mounir Lamouri wrote:
> Maybe you could move this into `PointerLockController::requestPointerLock`?

Done. Is that just to keep code out of Element.cpp?

https://codereview.chromium.org/1578263006/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/UseCounter.cpp (right):

https://codereview.chromium.org/1578263006/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/UseCounter.cpp:740: if (parent &&
!securityOrigin->canAccess(parent->securityContext()->securityOrigin()))
On 2016/01/19 17:54:48, Mounir Lamouri wrote:
> canAccess() is not exactly "cross origin". Maybe this should be specified in
the
> header.

Done. Actually you made me realise that this code isn't good enough. We want to
check if the frame has access into the top level document. It may be able to
script into its parent but the parent may be a different origin to the top level
document.

https://codereview.chromium.org/1578263006/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/mediastream/UserMediaRequest.cpp (right):

https://codereview.chromium.org/1578263006/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediastream/UserMediaRequest.cpp:128:
UseCounter::countCrossOriginIframe(*document,
UseCounter::GetUserMediaSecureOriginIframe);
On 2016/01/19 17:54:48, Mounir Lamouri wrote:
> Should you add a GetUserMediaInsecureOriginIframe like you did for
Geolocation?

It's not very useful (for us at least) because it no longer works on http on
ToT. If you're interested I can add it as well.

https://codereview.chromium.org/1578263006/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/notifications/Notification.cpp (right):

https://codereview.chromium.org/1578263006/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/notifications/Notification.cpp:328: // Measure
usage in cross-origin iframes.
On 2016/01/19 17:54:48, Mounir Lamouri wrote:
> Why do we need to know that? If we want to know usage, requestPermission() is
> going to add noise.

I tried to make what we're counting consistent across permissions. The question
is how do we define "usage". Is it:
1) when a site tries to use an API even if the user doesn't give permission. 
2) only when a site actually uses an API after it has been granted.

I chatted to Ben briefly about this - both seem interesting and I'd like to add
stats for both, but this CL was intended to count (1).

Notifications is an odd case. With other permission APIs, the permission request
is a part of the API flow. e.g. when you call geolocation, the the user will get
prompted with the call. Same with midi - you have to call requestMIDIAccess to
get access to the API, which will trigger a prompt. So with those APIs, we're
really currently counting API usage regardless of whether the user gives
permission.

With notifications, requesting the permission is a separate function. The site
could try to request the permission, but the user could deny it, so they would
never get to calling "new Notification" (which is what the above stat in this
file is counting). So if we didn't have this here, we would really only be
counting (2).

Does this make sense? I'm open to other suggestions.

[raymes, 2016-01-20 00:14:21]

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

[commit-bot: I haz the power, 2016-01-20 00:16:46]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1578263006/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1578263006/220001

[commit-bot: I haz the power, 2016-01-20 01:52:37]

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

[commit-bot: I haz the power, 2016-01-20 01:52:37]

Dry run: This issue passed the CQ dry run.

[raymes, 2016-01-20 06:29:55]

Patchset #10 (id:220001) has been deleted

[raymes, 2016-01-20 06:30:09]

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

[commit-bot: I haz the power, 2016-01-20 06:30:40]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1578263006/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1578263006/240001

[commit-bot: I haz the power, 2016-01-20 08:43:25]

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

[commit-bot: I haz the power, 2016-01-20 08:43:26]

Dry run: This issue passed the CQ dry run.

[mlamouri (slow - plz ping), 2016-01-21 13:53:18]

Regarding notifications permissions, I would tend to think that it would be
better to record permission requests from the permissions code (don't we already
do that?). Given that we count Notification usage in secure/insecure origins for
top frames only based on `new Notification`, I think we shouldn't count
permissions requests explicitely and if we have the two information separated,
we can still make something with them.

Regarding, getUserMedia, I think we might want to check for insecure origins
usage, the same way we record regular usage from insecure origins. Doesn't cost
much.

lgtm with the these two comments (above) applied.

https://codereview.chromium.org/1578263006/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/1578263006/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Element.cpp:3044:
UseCounter::countCrossOriginIframe(document(),
UseCounter::ElementRequestPointerLockIframe);
On 2016/01/20 at 00:13:12, raymes wrote:
> On 2016/01/19 17:54:48, Mounir Lamouri wrote:
> > Maybe you could move this into `PointerLockController::requestPointerLock`?
> 
> Done. Is that just to keep code out of Element.cpp?

Yes.

https://codereview.chromium.org/1578263006/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/mediastream/UserMediaRequest.cpp (right):

https://codereview.chromium.org/1578263006/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediastream/UserMediaRequest.cpp:128:
UseCounter::countCrossOriginIframe(*document,
UseCounter::GetUserMediaSecureOriginIframe);
On 2016/01/20 at 00:13:12, raymes wrote:
> On 2016/01/19 17:54:48, Mounir Lamouri wrote:
> > Should you add a GetUserMediaInsecureOriginIframe like you did for
Geolocation?
> 
> It's not very useful (for us at least) because it no longer works on http on
ToT. If you're interested I can add it as well.

You do it for geolocation. I think you should do it for gUM unless we have a
good reason not to.

[raymes, 2016-01-27 06:57:51]

https://codereview.chromium.org/1578263006/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/mediastream/UserMediaRequest.cpp (right):

https://codereview.chromium.org/1578263006/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediastream/UserMediaRequest.cpp:128:
UseCounter::countCrossOriginIframe(*document,
UseCounter::GetUserMediaSecureOriginIframe);
On 2016/01/21 13:53:18, Mounir Lamouri wrote:
> On 2016/01/20 at 00:13:12, raymes wrote:
> > On 2016/01/19 17:54:48, Mounir Lamouri wrote:
> > > Should you add a GetUserMediaInsecureOriginIframe like you did for
> Geolocation?
> > 
> > It's not very useful (for us at least) because it no longer works on http on
> ToT. If you're interested I can add it as well.
> 
> You do it for geolocation. I think you should do it for gUM unless we have a
> good reason not to.

Done.

[raymes, 2016-01-27 06:59:56]

The CQ bit was checked by raymes@chromium.org

[raymes, 2016-01-27 06:59:57]

The patchset sent to the CQ was uploaded after l-g-t-m from timloh@chromium.org,
mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/1578263006/#ps260001
(title: " ")

[commit-bot: I haz the power, 2016-01-27 07:00:04]

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

[commit-bot: I haz the power, 2016-01-27 08:22:04]

Committed patchset #11 (id:260001)

[commit-bot: I haz the power, 2016-01-27 08:23:15]

Description was changed from

==========
Add metrics for usage of permission features from cross-origin iframes in blink

This CL adds metrics to measure the usage of features that require permissions
in Chrome when they are used in cross-origin iframes only. In particular we only
want to count an iframe if it doesn't have scripting access into the top level
document. A new UseCounter::countCrossOriginIframe function has been added which
will only count this type of usage.

Note that this intends to count cases when a site tries to use an API whether or
not the permission is granted by the user.

BUG=444744
==========

to

==========
Add metrics for usage of permission features from cross-origin iframes in blink

This CL adds metrics to measure the usage of features that require permissions
in Chrome when they are used in cross-origin iframes only. In particular we only
want to count an iframe if it doesn't have scripting access into the top level
document. A new UseCounter::countCrossOriginIframe function has been added which
will only count this type of usage.

Note that this intends to count cases when a site tries to use an API whether or
not the permission is granted by the user.

BUG=444744

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

[commit-bot: I haz the power, 2016-01-27 08:23:16]

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