Closed Bug 1259087 Opened 4 years ago Closed 3 years ago

Add Windows sandboxing information to Telemetry

Categories

(Core :: Security: Process Sandboxing, defect)

All
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox50 --- fixed
firefox51 --- fixed

People

(Reporter: jimm, Assigned: bobowen)

References

Details

(Whiteboard: sbwc1)

Attachments

(1 file, 2 obsolete files)

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.
Assignee: nobody → jmathies
Assignee: jmathies → nobody
Assignee: nobody → bobowen.code
Status: NEW → ASSIGNED
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
Attachment #8781069 - Flags: review?(gfritzsche)
Attachment #8781069 - Flags: feedback?(benjamin)
Opened the following issue to get the added to the main ping schema:
https://github.com/mozilla-services/mozilla-pipeline-schemas/issues/11
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.
Attachment #8781069 - Flags: review?(gfritzsche) → feedback+
Thanks Georg, I've not used reSructuredText before, hopefully I've not messed up the syntax.

MozReview-Commit-ID: 8Irs0qvg8I9
Attachment #8781079 - Flags: review?(gfritzsche)
Attachment #8781079 - Flags: feedback?(benjamin)
Attachment #8781069 - Attachment is obsolete: true
Attachment #8781069 - Flags: feedback?(benjamin)
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.
Attachment #8781079 - Flags: review?(gfritzsche) → review+
Duplicate of this bug: 1259088
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.
Attachment #8781079 - Flags: feedback?(benjamin) → feedback-
(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?
Flags: needinfo?(benjamin)
"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.
Flags: needinfo?(benjamin)
MozReview-Commit-ID: 8Irs0qvg8I9
Attachment #8781079 - Attachment is obsolete: true
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.
Attachment #8786773 - Flags: review+
Pushed by bobowencode@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0eeaca87286
Add content process sandbox level to Telemetry Environment. r=gfritzsche
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?
Flags: needinfo?(gfritzsche)
Yes, we don't need to update the schema for a new user pref.
Flags: needinfo?(gfritzsche)
https://hg.mozilla.org/mozilla-central/rev/e0eeaca87286
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
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
Attachment #8786773 - Flags: approval-mozilla-aurora?
Comment on attachment 8786773 [details] [diff] [review]
Add content process sandbox level to Telemetry Environment

Makes sense, Aurora50+
Attachment #8786773 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.