1205156 - Add telemetry to measure how often getUserMedia is used over non-secure origins

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect)

Description

[Richard Barnes [:rbarnes]]

It would be nice to be able to turn off gUM support for non-secure origins.  We should measure how often that happens.

Comment 1

[Richard Barnes [:rbarnes]]

Attachment: bug-1205156.0.patch

Here's a first cut.  Note that results will not show up in e10s mode, since they're being collected in the child process.

mt: Does that look about right for how to determine "secure origin"?  Who should review this for real?

vladan: Thoughts?  Have I finally learned your lessons from my last few telemetry patches?  :)

Comment 2

[Nils Ohlmeier [:drno]]

Comment on attachment 8661592 [details] [diff] [review]
bug-1205156.0.patch

Review of attachment 8661592 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/Navigator.cpp
@@ +1382,5 @@
> +  nsCString origin;
> +  nsresult rv = principal->GetOrigin(origin);
> +  if (NS_SUCCEEDED(rv)) {
> +    printf("\n\n>>> got origin\n\n");
> +    if (origin.Find("http:") == 0) {

What about ftp:// and other schemes? Although I'm not sure if you can load a HTML or JS file which actually invokes gUM.
I think it might be better to search for the secure origin and let everything else fall into non-secure case. BTW juberti had to correct after his presentation that he should have said "secure origin" and not just "https", as the "secure origin" in their case will include localhost always as secure origin.

::: toolkit/components/telemetry/Histograms.json
@@ +6465,5 @@
>      "high": "30",
>      "n_buckets": "29",
>      "description": "The number of times AddIceCandidate failed on a given PeerConnection, given that ICE failed."
>    },
> +  "GET_USER_MEDIA_SECURE_ORIGIN": {

I would prefer to have a WEBRTC_ prefix here, as I think/expect that this will fall into the WebRTC teams responsibility.

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8661592 [details] [diff] [review]
bug-1205156.0.patch

Review of attachment 8661592 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/Navigator.cpp
@@ +1382,5 @@
> +  nsCString origin;
> +  nsresult rv = principal->GetOrigin(origin);
> +  if (NS_SUCCEEDED(rv)) {
> +    printf("\n\n>>> got origin\n\n");
> +    if (origin.Find("http:") == 0) {

!http != secure, as nils says.

Also echoing what nils said, per MediaManager::GetUserMedia(), privileged is set for IsCallerChrome() *or* a document source of about:loopconversation.  We track isHTTPS independently of privileged.  So secure would be (privileged || isHTTPS) I think.  Note that "loaded from localhost" would *not* be captured by that, but we also don't currently consider that secure either (at least in MediaManager) which can be annoying to devs testing screensharing code (which isn't allowed from insecure sources).

Really, this should all move there and it gets simpler.

Comment 4

[Richard Barnes [:rbarnes]]

Attachment: bug-1205156.1.patch

Thanks for the feedback, Nils and Randell.  I think I've hit most of your points in this patch:

* Adds WEBRTC_ prefix to telemetry key
* Moves telemetry to MediaManager::GetUserMedia() and uses the security flags there
* ... and adds a flag for 'isLocalhost'

So now the telemetry will collect several bins:
* Privileged
* HTTPS
* Non-HTTPS localhost
* Everything else

We've got 10 bins, so if we think of other distinctions, we can split them out later.

Comment 5

[Vladan Djeric (:vladan)]

Comment on attachment 8661788 [details] [diff] [review]
bug-1205156.1.patch

Review of attachment 8661788 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/telemetry/Histograms.json
@@ +6467,5 @@
>      "description": "The number of times AddIceCandidate failed on a given PeerConnection, given that ICE failed."
>    },
> +  "WEBRTC_GET_USER_MEDIA_SECURE_ORIGIN": {
> +    "alert_emails": ["[e-mail]"],
> +    "expires_in_version": "50",

I prefer shorter collection durations for opt-out histograms, but if you need a year's worth of data and you're going to monitor it, then it's ok

Comment 6

[Vladan Djeric (:vladan)]

> Note that results will not show up in e10s mode, since they're being collected in the child process.

The child-process results will show up in the dashboard. Do you mean they don't show up in about:telemetry? We'll fix that soon

Comment 7

[Richard Barnes [:rbarnes]]

(In reply to Vladan Djeric (:vladan) -- please needinfo! from comment #6)
> > Note that results will not show up in e10s mode, since they're being collected in the child process.
> 
> The child-process results will show up in the dashboard. Do you mean they
> don't show up in about:telemetry? We'll fix that soon

Yeah, I meant about:telemetry.  That's what I use for my manual testing of these patches.

Comment 8

[Richard Barnes [:rbarnes]]

(In reply to Vladan Djeric (:vladan) -- please needinfo! from comment #5)
> Comment on attachment 8661788 [details] [diff] [review]
> bug-1205156.1.patch
> 
> Review of attachment 8661788 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/telemetry/Histograms.json
> @@ +6467,5 @@
> >      "description": "The number of times AddIceCandidate failed on a given PeerConnection, given that ICE failed."
> >    },
> > +  "WEBRTC_GET_USER_MEDIA_SECURE_ORIGIN": {
> > +    "alert_emails": ["[e-mail]"],
> > +    "expires_in_version": "50",
> 
> I prefer shorter collection durations for opt-out histograms, but if you
> need a year's worth of data and you're going to monitor it, then it's ok

This is another one of those "need to know when we can turn off the non-secure stuff" metrics, so there is a fair chance we'll need a year of data.

Comment 9

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8661788 [details] [diff] [review]
bug-1205156.1.patch

Review of attachment 8661788 [details] [diff] [review]:
-----------------------------------------------------------------

Unsure about localhost test.

::: dom/media/MediaManager.cpp
@@ +1579,5 @@
>    aResult.Assign(NS_ConvertUTF8toUTF16(buffer));
>    return NS_OK;
>  }
>  
> +enum GetUserMediaSecurityState {

nit: could this be 'enum class' and/or moved to file-scope or class-scope? With unified builds we end up with lots of cruft out here.

@@ +1640,5 @@
>    docURI->SchemeIs("https", &isHTTPS);
> +  nsCString host;
> +  nsresult rv = docURI->GetHost(host);
> +  bool isLocalhost = NS_SUCCEEDED(rv) &&
> +                     host.LowerCaseEqualsLiteral("localhost");

Will this catch all localhost permutations, IPv6 etc? [1] Would using aDocURI->EqualsExceptRef like IsLoop() here does be an approach? 

[1] http://mxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerManager.cpp?rev=21b8c95e507a&mark=1441-1443#1434

@@ +1644,5 @@
> +                     host.LowerCaseEqualsLiteral("localhost");
> +
> +  // Record telemetry about whether the source of the call was secure, i.e.,
> +  // privileged or HTTPS.  We may handle other cases
> +  if (privileged) {

Wondering, do we want to separate loop from other privileged use? Bins are cheap

Comment 10

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Some observations (these may be fine, just making sure we're counting what we want):

- The telemetry does not track whether users are actually using camera/mic or not (gum success).

- This telemetry only tracks *calls* to gUM, even failed calls that never end up asking a user anything, either because the user has no camera/mic, has (in the case of https) denied the site access, or the site passes in constraints that can't be met. In the latter case, a picky site may request gum several times with different constraints until it succeeds, potentially skewing counts. Probably not common though.

- The telemetry includes screen-sharing (also accessed through gUM), an https-only feature, which will naturally skew numbers a bit toward https, but not a lot yet I suppose, since it's still a rare/white-listed feature.

Comment 11

[Richard Barnes [:rbarnes]]

Attachment: bug-1205156.2.patch

* enum -> enum class
* Made localhost definition consistent with ServiceWorkers
* Also check for "file" and "app" URIs, in case we want to handle these like SW

Comment 12

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8661832 [details] [diff] [review]
bug-1205156.2.patch

Review of attachment 8661832 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm.

::: toolkit/components/telemetry/Histograms.json
@@ +6470,5 @@
> +    "alert_emails": ["[e-mail]"],
> +    "expires_in_version": "50",
> +    "kind": "enumerated",
> +    "n_values": 15,
> +    "description": "Origins for getUserMedia calls (see bin mapping in MediaManager::GetUserMedia)",

I liked that you enumerated the values here earlier. Doesn't this description show up in the telemetry pages? Might be useful to keep that if there's no length limit here, in spite of the maintenance hassle.

Comment 13

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

The only thing would be if we should exclude https-only features like screen-sharing, since no-one wonders whether they're accessed from insecure origins or not. I think that would make for a better apples to apples comparison.

Comment 14

[Richard Barnes [:rbarnes]]

Attachment: bug-1205156.3.patch r=jib

Approval Request Comment
[Feature/regressing bug #]: Telemetry on use of getUserMedia from non-secure origins
[User impact if declined]: Delayed measurements
[Describe test coverage new/current, TreeHerder]: Manual
[Risks and why]: Low risk.  Very localized change.
[String/UUID change made/needed]: None.

Comment 15

[Martin Thomson [:mt:]]

Comment on attachment 8661592 [details] [diff] [review]
bug-1205156.0.patch

Review of attachment 8661592 [details] [diff] [review]:
-----------------------------------------------------------------

I think that you need to build a simple bool nsIPrincipal.IsSecure() that does this the right way.  And the right way isn't going to be easy: https://w3c.github.io/webappsec/specs/powerfulfeatures/#settings-secure  ckerschb knows how to test for this though.

Comment 16

[Martin Thomson [:mt:]]

Comment on attachment 8661863 [details] [diff] [review]
bug-1205156.3.patch r=jib

Review of attachment 8661863 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/MediaManager.cpp
@@ +1677,5 @@
> +                          (uint32_t) GetUserMediaSecurityState::Localhost);
> +  } else {
> +    Telemetry::Accumulate(Telemetry::WEBRTC_GET_USER_MEDIA_SECURE_ORIGIN,
> +                          (uint32_t) GetUserMediaSecurityState::Other);
> +  }

At least you could factor this enormous blob of telemetry stuff out.

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/380a1d59bbc3

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/380a1d59bbc3

Comment 19

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8661863 [details] [diff] [review]
bug-1205156.3.patch r=jib

Telemetry is good, taking it.

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f50999055675

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f50999055675