1819311 - Improve error reporting for process launches

Status: RESOLVED FIXED

(Core :: IPC, enhancement, P2)

Description

[:gerard-majax]

The rationale here is related to https://bugzilla.mozilla.org/show_bug.cgi?id=1789126#c22 ; tl;dr is:

• we hit a reject
• everything suggest this is because of GeckoChildProcessHost::Destroy that rejects when we are in the begining of starting a process
• we have IsShutdown() guarding on our utility process manager
• this depends on a boolean updated when xpcom-shutdown observer is delivered to us
• the test reproducing are tests where I already spotted raciness around starting a process when shutting down

So my take is we enter a small window of possible race between the GeckoChildProcessHost::Destroy and the moment we receive the observer, hence we get rejected by Destroy but we still dont know we are in shutdown. Moving to AppShutdown would, I hope, avoid that.

Comment 1

[:gerard-majax]

Attachment: Bug 1819311 - Collect errors on process launch r?nika!,bobowen!

Comment 2

[Nika Layzell [:nika] (ni? for response)]

I'm not convinced that this would do anything to improve the situation from https://bugzilla.mozilla.org/show_bug.cgi?id=1789126#c21. From looking at the stack you can see that we're not yet in proper XPCOM shutdown, so we can't have passed either xpcom-shutdown or xpcom-will-shutdown yet.

I think whatever failure is happening here is probably unrelated to shutdown. There are many different reasons why child process startup could fail (e.g. due to the binary being moved/replaced, among probably other things) so we should probably change the code to handle failure here, rather than diagnostic asserting.

Comment 3

[:gerard-majax]

So there's already some data collection on subprocess launch failures at https://searchfox.org/mozilla-central/rev/aa3ccd258b64abfd4c5ce56c1f512bc7f65b844c/ipc/glue/GeckoChildProcessHost.cpp#776-779 and it should cover what we want to look at, except maybe we lack of actual error message (but I could only see usages of LaunchError{} around, with LaunchError being an empty struct. I have however not been able to see the telemetry data so far.

Comment 4

[:gerard-majax]

Ok, there's some telemetry: https://telemetry.mozilla.org/new-pipeline/dist.html#!compare=child&cumulative=0&end_date=2023-03-02&include_spill=0&keys=tab!utility!rdd!__none__&max_channel_version=beta%252F111&measure=SUBPROCESS_LAUNCH_FAILURE&min_channel_version=nightly%252F100&processType=*&product=Firefox&sanitize=1&sort_by_value=0&sort_keys=submissions&start_date=2023-02-14&table=0&trim=1&use_submission_date=0

It does indeed show a few Utility failures to start.

Comment 5

[:gerard-majax]

Jens, on bug 1795821 you mention tracking better the kind error failure Which errors are propagated under which conditions by the async promise chain we have here and if there are recoverable conditions we might be able to distinguish and handle better or at least to notify to the user. So far looking at GeckoChildProcessHost I see mostly opaque LaunchError, is fixing this was what you implied ?

Comment 6

[Jens Stutte [:jstutte]]

I think I wanted to say something on the lines of:

1. check if we understand which conditions can lead to an error
2. check if those have distinct error codes/messages
3. verify if some of those are potentially avoidable (like checking something before launching)
4. verify if some of those are potentially recoverable (like retrying)
5. check then if all callers are checking well for error conditions and how they react and if we should tell something to the user (if we are not in shutdown)

So having something better than opaque LaunchError is definitely an improvement in that direction, as you would touch at least 1., 2. and a bit of 5. of the above list, yeah.

(this would alter the scope of this bug quite heavily, btw)

Comment 7

[:gerard-majax]

Attachment: WIP: Bug 1819311 - WIP Linux

Depends on D171226

Comment 8

[:gerard-majax]

Attachment: WIP: Bug 1819311 - WIP macOS

Depends on D172433

Comment 9

[:gerard-majax]

Attachment: data review

Comment 10

[Jan-Erik Rediger [:janerik]]

Comment on attachment 9326644 [details]
data review

I'm not a data steward, but chutten is and can take care of it here.

Comment 11

[:gerard-majax]

Attachment: Bug 1819311 - Limit hard crash when utility process is not starting r?nika!

Comment 12

[Chris H-C :chutten]

Comment on attachment 9326644 [details]
data review

DATA COLLECTION REVIEW RESPONSE:

Is there or will there be documentation that describes the schema for the ultimate data set available publicly, complete and accurate?

Yes.

Is there a control mechanism that allows the user to turn the data collection on and off?

Yes. This collection can be controlled through Firefox's Preferences.

If the request is for permanent data collection, is there someone who will monitor the data over time?

Yes, Alexandre Lissy is responsible.

Using the category system of data types on the Mozilla wiki, what collection type of data do the requested measurements fall under?

Category 1, Technical.

Is the data collection request for default-on or default-off?

Default on for all channels.

Does the instrumentation include the addition of any new identifiers?

No.

Is the data collection covered by the existing Firefox privacy notice?

Yes.

Does the data collection use a third-party collection tool?

No.

---

Result: datareview+

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/654ce2247015
Limit hard crash when utility process is not starting r=nika
https://hg.mozilla.org/integration/autoland/rev/6ef392f0f69a
Collect errors on process launch r=nika,geckoview-reviewers,owlish

Comment 14

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

https://hg.mozilla.org/mozilla-central/rev/654ce2247015
https://hg.mozilla.org/mozilla-central/rev/6ef392f0f69a