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

RESOLVED FIXED in Firefox 42

Status

()

defect
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: rbarnes, Assigned: rbarnes)

Tracking

unspecified
mozilla43
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 fixed, firefox43 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

It would be nice to be able to turn off gUM support for non-secure origins.  We should measure how often that happens.
Posted patch bug-1205156.0.patch (obsolete) — Splinter Review
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?  :)
Attachment #8661592 - Flags: feedback?(vladan.bugzilla)
Attachment #8661592 - Flags: feedback?(martin.thomson)
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 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.
Attachment #8661592 - Flags: feedback-
Posted patch bug-1205156.1.patch (obsolete) — Splinter Review
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.
Attachment #8661592 - Attachment is obsolete: true
Attachment #8661592 - Flags: feedback?(vladan.bugzilla)
Attachment #8661592 - Flags: feedback?(martin.thomson)
Attachment #8661788 - Flags: review?(rjesup)
Attachment #8661788 - Flags: feedback?(vladan.bugzilla)
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": ["seceng@mozilla.org"],
> +    "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
Attachment #8661788 - Flags: feedback?(vladan.bugzilla) → feedback+
> 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
(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.
(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": ["seceng@mozilla.org"],
> > +    "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 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
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.
Posted patch bug-1205156.2.patch (obsolete) — Splinter Review
* 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
Attachment #8661788 - Attachment is obsolete: true
Attachment #8661788 - Flags: review?(rjesup)
Attachment #8661832 - Flags: review?(jib)
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": ["seceng@mozilla.org"],
> +    "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.
Attachment #8661832 - Flags: review?(jib) → review+
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.
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.
Attachment #8661832 - Attachment is obsolete: true
Attachment #8661863 - Flags: review+
Attachment #8661863 - Flags: approval-mozilla-aurora?
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.
Attachment #8661592 - Flags: feedback-
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.
https://hg.mozilla.org/mozilla-central/rev/380a1d59bbc3
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Assignee: nobody → rlb
Comment on attachment 8661863 [details] [diff] [review]
bug-1205156.3.patch r=jib

Telemetry is good, taking it.
Attachment #8661863 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Blocks: 1346357
You need to log in before you can comment on or make changes to this bug.