Closed
Bug 1395952
Opened 7 years ago
Closed 7 years ago
Improve telemetry for failed launch of Windows sandboxed process.
Categories
(Core :: Security: Process Sandboxing, enhancement, P1)
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: bobowen, Assigned: bobowen)
References
(Depends on 1 open bug)
Details
(Whiteboard: sb+)
Attachments
(2 files, 2 obsolete files)
We currently log the sandbox::ResultCode when a windows sandboxed child process fails to launch. It would be more useful if we had the Gecko process type and Windows error as well. We could crash Firefox to get a report, but this would only make sense for the content process and would be bad if any of the launch issues can be intermittent. Instead I'm going to change the histogram which logs the ResultCode enum to be keyed and use the string representation of the process type and the Windows error code, which is usually populated from GetLastError by the sandbox code. In theory using the windows error could give a large number of keys, but in practice there will probably be a limited number that occur.
Assignee | ||
Comment 1•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a23f391341fd4a270db29225d2167c5b9927264d
Assignee | ||
Comment 2•7 years ago
|
||
Attachment #8904180 -
Flags: review?(jmathies)
Assignee | ||
Comment 3•7 years ago
|
||
Assignee | ||
Comment 4•7 years ago
|
||
Comment on attachment 8904180 [details] [diff] [review] Enhance telemetry for failed launch of Windows sandboxed process by process type/error code key This is a change to an existing probe, but I also want to make it permanent, so I've filled in a new form which is attached to the bug. I've changed the name in case the changes cause a conflict with the old probe.
Attachment #8904180 -
Flags: feedback?(rweiss)
Comment 5•7 years ago
|
||
Comment on attachment 8904180 [details] [diff] [review] Enhance telemetry for failed launch of Windows sandboxed process by process type/error code key Review of attachment 8904180 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/sandbox/win/src/sandboxbroker/sandboxBroker.cpp @@ +207,5 @@ > &last_warning, &last_error, &targetInfo); > if (sandbox::SBOX_ALL_OK != result) { > + nsAutoCString key; > + key.AppendASCII(XRE_ChildProcessTypeToString(aProcessType)); > + key.AppendLiteral("/0x"); what is this literal separator?
Attachment #8904180 -
Flags: review?(jmathies) → review+
Assignee | ||
Comment 6•7 years ago
|
||
Thanks jimm. (In reply to Jim Mathies [:jimm] from comment #5) ... > > + key.AppendLiteral("/0x"); > > what is this literal separator? Just a slash followed by 0x to signify that the error code after that is in hex.
Assignee | ||
Comment 7•7 years ago
|
||
Only one telemetry accumlation will occur for each key per session.
Attachment #8906560 -
Flags: review?(jmathies)
Assignee | ||
Updated•7 years ago
|
Attachment #8904180 -
Attachment is obsolete: true
Attachment #8904180 -
Flags: feedback?(rweiss)
Assignee | ||
Comment 8•7 years ago
|
||
Comment on attachment 8906560 [details] [diff] [review] Enhance telemetry for failed launch of Windows sandboxed process by process type/error code key Only change here is to only count each process type/error code key combination once per session.
Attachment #8906560 -
Flags: feedback?(rweiss)
Updated•7 years ago
|
Attachment #8906560 -
Flags: review?(jmathies) → review+
Comment 9•7 years ago
|
||
Comment on attachment 8906560 [details] [diff] [review] Enhance telemetry for failed launch of Windows sandboxed process by process type/error code key Review of attachment 8906560 [details] [diff] [review]: ----------------------------------------------------------------- 1) Is there documentation that describes the schema for the ultimate data set available publicly, complete and accurate? Yes, this will have standard Telemetry documentation. 2) Is there a control mechanism that allows the user to turn the data collection on and off? Yes, standard Telemetry opt-out preference 3) If the request is for permanent data collection, is there someone who will monitor the data over time? Bob Owen will monitor permanently. 4) Using the category system of data types on the Mozilla wiki, what collection type of data do the requested measurements fall under? Category 1. 5) Is the data collection default-on or default-off? Default on, all Windows users with e10s enabled. 6) Does the instrumentation include the addition of any new identifiers? No 7) Is the data collection covered by the existing Firefox privacy notice? Yes 8) Does there need to be a check-in in the future to determine whether to renew the data? No
Attachment #8906560 -
Flags: feedback?(rweiss) → feedback+
Assignee | ||
Comment 10•7 years ago
|
||
Just realised that I mistakenly said this would only be captured for e10s users. These processes get launched even for non-e10s, so it would also be captured if these failed. Corrected request doc attached and for the review below. (In reply to Rebecca Weiss from comment #9) > Comment on attachment 8906560 [details] [diff] [review] > Enhance telemetry for failed launch of Windows sandboxed process by process > type/error code key > > Review of attachment 8906560 [details] [diff] [review]: > ----------------------------------------------------------------- ... > 5) Is the data collection default-on or default-off? Default on, all Windows users running Firefox who have sandboxed child process launch failures.
Attachment #8904181 -
Attachment is obsolete: true
Comment 11•7 years ago
|
||
Pushed by bobowencode@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/65261dd25999 Enhance telemetry for failed launch of Windows sandboxed process by process type/error code key. r=jimm, data-r=rweiss
Comment 12•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/65261dd25999
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•