1395952 - Improve telemetry for failed launch of Windows sandboxed process.

Status: RESOLVED FIXED

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

Description

[Bob Owen (:bobowen)]

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 1

[Bob Owen (:bobowen)]

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

Comment 2

[Bob Owen (:bobowen)]

Attachment: Enhance telemetry for failed launch of Windows sandboxed process by process type/error code key

Comment 3

[Bob Owen (:bobowen)]

Attachment: Request for Data Collection Review for bug 1395952.pdf

Comment 4

[Bob Owen (:bobowen)]

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.

Comment 5

[Jim Mathies [:jimm]]

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?

Comment 6

[Bob Owen (:bobowen)]

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.

Comment 7

[Bob Owen (:bobowen)]

Attachment: Enhance telemetry for failed launch of Windows sandboxed process by process type/error code key

Only one telemetry accumlation will occur for each key per session.

Comment 8

[Bob Owen (:bobowen)]

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.

Comment 9

[[e-mail]]

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

Comment 10

[Bob Owen (:bobowen)]

Attachment: Request for Data Collection Review for bug 1395952.pdf

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.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
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

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/65261dd25999