Closed Bug 1395952 Opened 2 years ago Closed 2 years ago

Improve telemetry for failed launch of Windows sandboxed process.

Categories

(Core :: Security: Process Sandboxing, enhancement, P1)

All
Windows
enhancement

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.
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 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+
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.
Only one telemetry accumlation will occur for each key per session.
Attachment #8906560 - Flags: review?(jmathies)
Attachment #8904180 - Attachment is obsolete: true
Attachment #8904180 - Flags: feedback?(rweiss)
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)
Attachment #8906560 - Flags: review?(jmathies) → review+
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+
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
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
https://hg.mozilla.org/mozilla-central/rev/65261dd25999
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Duplicate of this bug: 1353734
Depends on: 1470288
You need to log in before you can comment on or make changes to this bug.