1259087 - Add Windows sandboxing information to Telemetry

Status: RESOLVED FIXED

(Core :: Security: Process Sandboxing, defect)

Description

[Jim Mathies [:jimm]]

Add basic information about the sandbox settings, either to the telemetry environment or as a probe. We'd prefer environment but not if it holds up our rollout. We can always morph a probe into that later.

Comment 1

[Bob Owen (:bobowen)]

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

Comment 2

[Bob Owen (:bobowen)]

Attachment: Add content process sandbox level to Telemetry Environment

I've followed the patch from bug 1249845 for the e10s cohort setting.

Flagging bsmedberg for feedback re data collection approval.

MozReview-Commit-ID: 8Irs0qvg8I9

Comment 3

[Bob Owen (:bobowen)]

Opened the following issue to get the added to the main ping schema:
https://github.com/mozilla-services/mozilla-pipeline-schemas/issues/11

Comment 4

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8781069 [details] [diff] [review]
Add content process sandbox level to Telemetry Environment

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

::: toolkit/components/telemetry/docs/environment.rst
@@ +44,5 @@
>          },
>          searchCohort: <string>, // optional, contains an identifier for any active search A/B experiments
>          e10sEnabled: <bool>, // whether e10s is on, i.e. browser tabs open by default in a different process
>          e10sCohort: <string>, // which e10s cohort was assigned for this user
> +        contentSandboxLevel: <number>, // content process sandbox level; meaning is OS dependent, 0 means not sandboxed on all OS

Please add a paragraph further down that details the meaning or points to further information.

Comment 5

[Bob Owen (:bobowen)]

Attachment: Add content process sandbox level to Telemetry Environment

Thanks Georg, I've not used reSructuredText before, hopefully I've not messed up the syntax.

MozReview-Commit-ID: 8Irs0qvg8I9

Comment 6

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8781079 [details] [diff] [review]
Add content process sandbox level to Telemetry Environment

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

It would be nice if those values were easier to look up, but this works.

Comment 8

[Benjamin Smedberg]

Comment on attachment 8781079 [details] [diff] [review]
Add content process sandbox level to Telemetry Environment

Talked about this on IRC: we don't need a new key here, since userPrefs can control this.

Add this to http://searchfox.org/mozilla-central/source/toolkit/components/telemetry/TelemetryEnvironment.jsm#110 and any non-default (user-changed) values will be recorded automatically. data-review+ on that change, but not this one.

Comment 9

[Bob Owen (:bobowen)]

(In reply to Benjamin Smedberg [:bsmedberg] from comment #8)

> TelemetryEnvironment.jsm#110 and any non-default (user-changed) values will
> be recorded automatically. data-review+ on that change, but not this one.

Just wanted to confirm something.
If we, for example, change the default for beta back to 0 (off) and use a system addon to set it back to 1 for a certain percentage.
Am I correct in thinking that this would count as user-changed and appear in the telemetry environment?

Comment 10

[Benjamin Smedberg]

"it depends": if you set this as a user pref, then yes. If you're talking about changing this as a default pref (via defaults/prefences) then no.

Comment 11

[Bob Owen (:bobowen)]

Attachment: Add content process sandbox level to Telemetry Environment

MozReview-Commit-ID: 8Irs0qvg8I9

Comment 12

[Bob Owen (:bobowen)]

Comment on attachment 8786773 [details] [diff] [review]
Add content process sandbox level to Telemetry Environment

Carrying:
r=gfritzsche from comment 6.
data-review+ from bsmedberg in comment 8.

Comment 13

[Bob Owen (:bobowen)]

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

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0eeaca87286
Add content process sandbox level to Telemetry Environment. r=gfritzsche

Comment 15

[Bob Owen (:bobowen)]

Given that this was done using the user prefs section in the end, can I close https://github.com/mozilla-services/mozilla-pipeline-schemas/issues/11 or does it need changing to allow for the new user pref in the list?

Comment 16

[Georg Fritzsche [:gfritzsche]]

Yes, we don't need to update the schema for a new user pref.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e0eeaca87286

Comment 18

[Bob Owen (:bobowen)]

Comment on attachment 8786773 [details] [diff] [review]
Add content process sandbox level to Telemetry Environment

Approval Request Comment
[Feature/regressing bug #]:
First content sandboxing currently set to roll out with Fx50, so it would be good to get this pref in the Telemetry environment.

[User impact if declined]:
It may reduce our ability to diagnose issues.

[Describe test coverage new/current, TreeHerder]:
This just adds a new pref to a list that are already collected.
There are automated telemetry environment tests.

[Risks and why]:
Low - simple addition of the pref to an existing list.

[String/UUID change made/needed]:
None

Comment 19

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8786773 [details] [diff] [review]
Add content process sandbox level to Telemetry Environment

Makes sense, Aurora50+

Comment 20

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

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