1359124 - Add Telemetry for Light Sensor API

Status: RESOLVED INVALID

(Core :: DOM: Device Interfaces, enhancement, P1)

Description

[Tom Ritter [:tjr]]

Especially based on https://blog.lukaszolejnik.com/stealing-sensitive-browser-data-with-the-w3c-ambient-light-sensor-api/ it would be good to measure the usage of this feature.

Comment 1

[Andrew Overholt [:overholt]]

I think you'd want to add something around http://searchfox.org/mozilla-central/source/dom/system/nsDeviceSensors.cpp#368 (but I don't really know).

Comment 2

[Jonathan Kingston [:jkt] he/him]

I'd argue http://searchfox.org/mozilla-central/source/dom/system/nsDeviceSensors.cpp#170 is probably more useful as it will give us an indicator of how many sites use this feature, rather than how often it fires on those sites.

Willing to add some telemetry for this, it might be easier to just track all device sensors with this telemetry. Which as per my bug 1359076 is something we probably want.

I'm gonna take a stab at doing this and will report back. Please steal it if I'm slow at that.

Comment 3

[Jonathan Kingston [:jkt] he/him]

Attachment: Bug 1359124 - Adding in telemetry for generic sensor usage.

Review commit: https://reviewboard.mozilla.org/r/133356/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/133356/

Comment 4

[Jonathan Kingston [:jkt] he/him]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f7097ed474b6

Comment 5

[Tom Ritter [:tjr]]

Comment on attachment 8861371 [details]
Bug 1359124 - Adding in telemetry for generic sensor usage.

https://reviewboard.mozilla.org/r/133356/#review136250

::: dom/events/EventListenerManager.cpp:460
(Diff revision 1)
>  
>  void
> +EventListenerManager::SendTelemetry(int eventType, bool isSecure)
> +{
> +  if (isSecure) {
> +    eventType += 6;

This strategy is going to make it very painful to add a telemetry probe for any new sensor we add; since we'd have to make it have number 13; and change the clean addition logic...

I imagine Telemetry can handle negative enum values right? So how about insecure sensor events are negative and secure events are positive?

::: toolkit/components/telemetry/Histograms.json:826
(Diff revision 1)
>    },
> +  "DEVICE_SENSOR_ADD_EVENT": {
> +    "alert_emails": ["[e-mail]"],
> +    "expires_in_version": "60",
> +    "kind": "enumerated",
> +    "n_values": 14,

I'd probably be a little more conserative and make this like 20 or 26.

Comment 6

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8861371 [details]
Bug 1359124 - Adding in telemetry for generic sensor usage.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/133356/diff/1-2/

Comment 7

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8861371 [details]
Bug 1359124 - Adding in telemetry for generic sensor usage.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/133356/diff/2-3/

Comment 8

[Jonathan Kingston [:jkt] he/him]

Eh managed to lose my comment and publish two reviews instead.

Anyway Accumulate only accepts a uint so I added 10 instead as I can't see us adding 4 more sensor types any time soon (5 if we add a 0).

Comment 9

[Jonathan Kingston [:jkt] he/him]

Hey Francois,
Could I get a data review also?

Comment 10

[Tom Ritter [:tjr]]

Comment on attachment 8861371 [details]
Bug 1359124 - Adding in telemetry for generic sensor usage.

https://reviewboard.mozilla.org/r/133356/#review136328

::: dom/events/EventListenerManager.cpp:460
(Diff revision 2)
>  
>  void
> +EventListenerManager::SendTelemetry(int eventType, bool isSecure)
> +{
> +  if (isSecure) {
> +    eventType += 10;

I think you should make this +13 so the max of 26 is evenly distributed for secure/insecure. I doubt we'll add 4 more sensors, but we might add a sensor that has 4 or 5 triggers (orientation has 3...) 

Otherwise looks good to me. (Someone might tell you to turn magical constants into const or defines... but I won't.)

Comment 11

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8861371 [details]
Bug 1359124 - Adding in telemetry for generic sensor usage.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/133356/diff/2-3/

Comment 12

[François Marier [:francois]]

What exactly is the question you're trying to answer?

Is it this: For each type of sensor, what percentage of the requests to access them are over HTTPS?

Comment 13

[Jonathan Kingston [:jkt] he/him]

Both:
1. How often the Generic Sensor APIs are used over secure/non secure?
2. How many pages call the Generic Sensor APIs?

Perhaps it would be more accurate to answer 2. with moving the code into the enable sensor call here (I actually implemented it like this first but thought it would be neater here):

http://searchfox.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#13302

Currently 2. would be over counting as a page may attach numerous event handlers.

Comment 14

[François Marier [:francois]]

(In reply to Jonathan Kingston [:jkt] from comment #13)
> 1. How often the Generic Sensor APIs are used over secure/non secure?

I think that what you have will achieve this.

> 2. How many pages call the Generic Sensor APIs?

However, I don't think you'll get that data from the probe as it is.

I would suggest reaching out to folks on #telemetry to figure out how to do this properly.

Comment 15

[Jonathan Kingston [:jkt] he/him]

Removing myself from assign right now as I spoke to telemetry and there wasn't a easy way to get the proportion of users seeing the APIs easily there wasn't an obvious answer I could see.

I'm not sure if anything has changed since I last looked into this bug though. However as this was just an interest rather than an area I am looking after it's safe to say I'm unlikely to have the time to dedicate on this right now.

Comment 16

[Andrew Overholt [:overholt]]

Is anyone still working on this? Are we still trying to remove this API?

Comment 17

[Anne (:annevk)]

The current plan is to disable this API without telemetry. I suspect that Frederik will get back to it once back from PTO. Once bug 1359076 is fixed (and it indeed ends up disabling this API) we can close this as INVALID or some such.

Comment 18

[Frederik Braun [:freddy]]

yep