Record whether Geolocation is accessed from a secure origin.

Record whether Geolocation is accessed from a secure origin.

BUG=352380
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=183149

[mlamouri (slow - plz ping), 2014-10-02 14:50:55]

mlamouri@chromium.org changed reviewers:
+ mvanouwerkerk@chromium.org

[Mike West, 2014-10-02 14:56:32]

mkwst@chromium.org changed reviewers:
+ mkwst@chromium.org

[Mike West, 2014-10-02 14:56:33]

LGTM. Thanks for taking care of this.

[Peter Beverloo, 2014-10-02 14:58:24]

peter@chromium.org changed reviewers:
+ peter@chromium.org

[Peter Beverloo, 2014-10-02 14:58:25]

https://codereview.chromium.org/620003005/diff/1/Source/modules/geolocation/G...
File Source/modules/geolocation/Geolocation.cpp (right):

https://codereview.chromium.org/620003005/diff/1/Source/modules/geolocation/G...
Source/modules/geolocation/Geolocation.cpp:146: if (!document)
When would there not be a document if we have the frame? We seem to assume in
other places (e.g. Geolocation::watchPosition) that the executionContext() is
alive. Can we ASSERT instead?

https://codereview.chromium.org/620003005/diff/1/Source/modules/geolocation/G...
Source/modules/geolocation/Geolocation.cpp:155: UseCounter::count(document,
counter);
I would much prefer this code to use SecurityOrigin::isSecure() based on the
document's URL. Right now, it would consider localhost URLs as being secure
either, which I don't think we should include in the UMA.

https://codereview.chromium.org/620003005/diff/1/Source/modules/geolocation/G...
File Source/modules/geolocation/Geolocation.h (right):

https://codereview.chromium.org/620003005/diff/1/Source/modules/geolocation/G...
Source/modules/geolocation/Geolocation.h:170: void RecordOriginTypeAccess();
nit: Should start with a lower-case character per the Blink style-guide.

[Michael van Ouwerkerk, 2014-10-02 15:03:45]

Thanks. I think part of the discussion was also about whether the permission
grant / reject rate was affected by origin security. This could be tracked
generically in the browser process for all permission requests for all APIs.
Would you like to tackle that as well? :-)

[Mike West, 2014-10-02 15:04:11]

https://codereview.chromium.org/620003005/diff/1/Source/modules/geolocation/G...
File Source/modules/geolocation/Geolocation.cpp (right):

https://codereview.chromium.org/620003005/diff/1/Source/modules/geolocation/G...
Source/modules/geolocation/Geolocation.cpp:155: UseCounter::count(document,
counter);
On 2014/10/02 14:58:24, Peter Beverloo wrote:
> I would much prefer this code to use SecurityOrigin::isSecure() based on the
> document's URL. Right now, it would consider localhost URLs as being secure
> either, which I don't think we should include in the UMA.

We're using canAccessFeature* for security checks (like ServiceWorker) now. If
that's the definition we're running with, it's what we should measure.

[Peter Beverloo, 2014-10-02 15:07:35]

On 2014/10/02 15:04:11, Mike West (OOO until 6th) wrote:
>
https://codereview.chromium.org/620003005/diff/1/Source/modules/geolocation/G...
> File Source/modules/geolocation/Geolocation.cpp (right):
> 
>
https://codereview.chromium.org/620003005/diff/1/Source/modules/geolocation/G...
> Source/modules/geolocation/Geolocation.cpp:155: UseCounter::count(document,
> counter);
> On 2014/10/02 14:58:24, Peter Beverloo wrote:
> > I would much prefer this code to use SecurityOrigin::isSecure() based on the
> > document's URL. Right now, it would consider localhost URLs as being secure
> > either, which I don't think we should include in the UMA.
> 
> We're using canAccessFeature* for security checks (like ServiceWorker) now. If
> that's the definition we're running with, it's what we should measure.

Do we consider every localhost URL to be a secure origin, even when using HTTP?
The UMA names explicitly focus on whether the origin is secure or not, so
perhaps we should settle on a more accurate name instead?

[Mike West, 2014-10-02 15:10:57]

On 2014/10/02 15:07:35, Peter Beverloo wrote:
> Do we consider every localhost URL to be a secure origin, even when using
HTTP?

Yes. Chrome has to trust the local machine. Loopback addresses, file:///
addresses, etc. are "secure" for purposes of this API call.

> The UMA names explicitly focus on whether the origin is secure or not, so
> perhaps we should settle on a more accurate name instead?

The mixed content spec calls them "authenticated origins". palmer@ calls them
"secure origins". Pick your poison. :)

[Peter Beverloo, 2014-10-02 15:12:58]

On 2014/10/02 15:10:57, Mike West (OOO until 6th) wrote:
> On 2014/10/02 15:07:35, Peter Beverloo wrote:
> > Do we consider every localhost URL to be a secure origin, even when using
> HTTP?
> 
> Yes. Chrome has to trust the local machine. Loopback addresses, file:///
> addresses, etc. are "secure" for purposes of this API call.
> 
> > The UMA names explicitly focus on whether the origin is secure or not, so
> > perhaps we should settle on a more accurate name instead?
> 
> The mixed content spec calls them "authenticated origins". palmer@ calls them
> "secure origins". Pick your poison. :)

OK, sounds good to me then! Thanks. (My other comments still stand.)

[Peter Beverloo, 2014-10-02 15:13:21]

lgtm since they're nits

[mlamouri (slow - plz ping), 2014-10-02 15:44:55]

https://codereview.chromium.org/620003005/diff/1/Source/modules/geolocation/G...
File Source/modules/geolocation/Geolocation.cpp (right):

https://codereview.chromium.org/620003005/diff/1/Source/modules/geolocation/G...
Source/modules/geolocation/Geolocation.cpp:146: if (!document)
On 2014/10/02 14:58:24, Peter Beverloo wrote:
> When would there not be a document if we have the frame? We seem to assume in
> other places (e.g. Geolocation::watchPosition) that the executionContext() is
> alive. Can we ASSERT instead?

Done.

https://codereview.chromium.org/620003005/diff/1/Source/modules/geolocation/G...
File Source/modules/geolocation/Geolocation.h (right):

https://codereview.chromium.org/620003005/diff/1/Source/modules/geolocation/G...
Source/modules/geolocation/Geolocation.h:170: void RecordOriginTypeAccess();
On 2014/10/02 14:58:24, Peter Beverloo wrote:
> nit: Should start with a lower-case character per the Blink style-guide.

Done.

[Michael van Ouwerkerk, 2014-10-02 15:47:26]

lgtm with nits

https://codereview.chromium.org/620003005/diff/20001/Source/modules/geolocati...
File Source/modules/geolocation/Geolocation.cpp (right):

https://codereview.chromium.org/620003005/diff/20001/Source/modules/geolocati...
Source/modules/geolocation/Geolocation.cpp:145: Document* doc = document();
Please don't use abbreviations for variables without good reason. Just call this
'document' - it's still short.

https://codereview.chromium.org/620003005/diff/20001/Source/modules/geolocati...
Source/modules/geolocation/Geolocation.cpp:148: // It is required but
canAccessFeatureRequiringSecureOrigin() but isn't
but but?

[Peter Beverloo, 2014-10-02 15:49:24]

https://codereview.chromium.org/620003005/diff/20001/Source/modules/geolocati...
File Source/modules/geolocation/Geolocation.cpp (right):

https://codereview.chromium.org/620003005/diff/20001/Source/modules/geolocati...
Source/modules/geolocation/Geolocation.cpp:148: // It is required but
canAccessFeatureRequiringSecureOrigin() but isn't
On 2014/10/02 15:47:26, Michael van Ouwerkerk wrote:
> but but?

s/canAccessFeatureRequiringSecureOrigin()/insecureOriginMsg/

[mlamouri (slow - plz ping), 2014-10-02 15:53:53]

nits fixed, PTAL

[mlamouri (slow - plz ping), 2014-10-02 15:54:02]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2014-10-02 15:55:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/620003005/40001

[commit-bot: I haz the power, 2014-10-02 15:55:13]

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

[commit-bot: I haz the power, 2014-10-02 15:55:14]

Failed to apply patch for Source/core/frame/UseCounter.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/frame/UseCounter.h
  Hunk #1 FAILED at 527.
  1 out of 1 hunk FAILED -- saving rejects to file
Source/core/frame/UseCounter.h.rej

Patch:       Source/core/frame/UseCounter.h
Index: Source/core/frame/UseCounter.h
diff --git a/Source/core/frame/UseCounter.h b/Source/core/frame/UseCounter.h
index
dd58eeff268663d0111953ea45ec6a6deeb3974b..d0c237f0f1e7efbf5997653235a64f74ddbbb342
100644
--- a/Source/core/frame/UseCounter.h
+++ b/Source/core/frame/UseCounter.h
@@ -527,6 +527,8 @@ public:
         ScreenOrientationType = 558,
         ScreenOrientationLock = 559,
         ScreenOrientationUnlock = 560,
+        GeolocationSecureOrigin = 561,
+        GeolocationInsecureOrigin = 562,
         // Add new features immediately above this line. Don't change assigned
         // numbers of any item, and don't reuse removed slots.
         // Also, run update_use_counter_feature_enum.py in
chromium/src/tools/metrics/histograms/

[mlamouri (slow - plz ping), 2014-10-02 16:11:23]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2014-10-02 16:11:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/620003005/40001

[commit-bot: I haz the power, 2014-10-02 17:19:38]

Committed patchset #3 (id:40001) as 183149