1165945 - Better handle a failure to start a content child process.

Status: RESOLVED WONTFIX

(Core :: DOM: Content Processes, defect, P3)

Description

[Bob Owen (:bobowen)]

On Windows, bug 1146874 fixed the problem where a failure to start a content process would cause the main process to crash.

Now we don't crash, but nothing gets rendered.

Should we handle this better, with retries or communication to the user, like when the content process crashes?

Should we be reporting this via the crash reporter?
Maybe the reporting should be common code in GeckoChildProcessHost.cpp

Comment 1

[Bob Owen (:bobowen)]

We should probably communicate (and capture with telemetry) this sort of child process start-up failure to the user before releasing e10s.

Comment 2

[Brad Lassey [:blassey] (use needinfo?)]

Bob seems like a printing to the console should suffice for now

Comment 3

[Brad Lassey [:blassey] (use needinfo?)]

In addition to logging to the console, can you add a telemetry probe so we can know if this is happening?

Comment 4

[Haik Aftandilian [:haik]]

Attachment: Adds a telemetry probe and console logging to record subprocess launch failures

Bob, is this what you had in mind?

Comment 5

[Bob Owen (:bobowen)]

Comment on attachment 8752907 [details] [diff] [review]
Adds a telemetry probe and console logging to record subprocess launch failures

Review of attachment 8752907 [details] [diff] [review]:
-----------------------------------------------------------------

This bug was originally about handling when the process fails and what happens after we return false from RunPerformAsyncLaunch.

I put some thoughts in the Description, but I'm not sure who decides how we should handle it and I don't think I'd be the right person to review it when we do.

A simple count like this of process type failures, is probably a good first probe, to see if there is a significant problem that we need to solve.
I think you have to get a data peer review for new probes (an f? to :bsmedberg is how the MDN page suggests you do this).

::: ipc/glue/GeckoChildProcessHost.cpp
@@ +564,5 @@
> +
> +  if (PerformAsyncLaunch(aExtraOpts, aArch)) {
> +    return true;
> +  } else {
> +    CHROMIUM_LOG(ERROR) << "Failed to launch " <<

Does this log to the console?

Comment 6

[Haik Aftandilian [:haik]]

(In reply to Bob Owen (:bobowen) from comment #5)
> Comment on attachment 8752907 [details] [diff] [review]
> Adds a telemetry probe and console logging to record subprocess launch
> failures
> 
> Review of attachment 8752907 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This bug was originally about handling when the process fails and what
> happens after we return false from RunPerformAsyncLaunch.
> 
> I put some thoughts in the Description, but I'm not sure who decides how we
> should handle it and I don't think I'd be the right person to review it when
> we do.

OK. I agree that a page like tab crashed would be ideal. Will look into that. Thanks.

> A simple count like this of process type failures, is probably a good first
> probe, to see if there is a significant problem that we need to solve.
> I think you have to get a data peer review for new probes (an f? to
> :bsmedberg is how the MDN page suggests you do this).

Wanted to check with you first, will get review from bsmedberg regarding the telemetry.

> ::: ipc/glue/GeckoChildProcessHost.cpp
> @@ +564,5 @@
> > +
> > +  if (PerformAsyncLaunch(aExtraOpts, aArch)) {
> > +    return true;
> > +  } else {
> > +    CHROMIUM_LOG(ERROR) << "Failed to launch " <<
> 
> Does this log to the console?

Yet to test on other platforms, but on OS X it does for debug and non-debug builds.

Comment 7

[Haik Aftandilian [:haik]]

Comment on attachment 8752907 [details] [diff] [review]
Adds a telemetry probe and console logging to record subprocess launch failures

f? for data collection review.

Comment 8

[Benjamin Smedberg]

Comment on attachment 8752907 [details] [diff] [review]
Adds a telemetry probe and console logging to record subprocess launch failures

Why is n_values 16? Do we really expect there to be 16 process types in the future?

Please document in the histogram "description" what the values mean. Either directly, or reference the enumeration type used. data-review=me with that fixed.

Comment 9

[Haik Aftandilian [:haik]]

Attachment: Adds a telemetry probe and console logging to record subprocess launch failures

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #8)
> Comment on attachment 8752907 [details] [diff] [review]
> Adds a telemetry probe and console logging to record subprocess launch
> failures
> 
> Why is n_values 16? Do we really expect there to be 16 process types in the
> future?

The GeckoProcessType macro goes from 0-5 now. It's conceivable that we might add more process types as we implement sandboxing. I've reduced this to 10 to leave room to add more.

> Please document in the histogram "description" what the values mean. Either
> directly, or reference the enumeration type used. data-review=me with that
> fixed.

Updated the histogram description to read "Counts the number of times launching a subprocess fails. Counts are by subprocess-type using the GeckoProcessType enum."

Comment 10

[Haik Aftandilian [:haik]]

I'm splitting off the telemetry and logging to bug 1275430 so that this bug can be used to improve the user experience when a content process fails once we determine how frequently that happens. When I commented out the code to start a content process to test the telemetry code, Firefox hung at launch and no UI ever was displayed.

Comment 11

[Sylvestre Ledru [:Sylvestre]]

Moving to p3 because no activity for at least 1 year(s).
See https://github.com/mozilla/bug-handling/blob/master/policy/triage-bugzilla.md#how-do-you-triage for more information

Comment 12

[BugBot [:suhaib / :marco/ :calixte]]

The severity field for this bug is relatively low, S3. However, the bug has 5 See Also bugs.
:jjalkanen, could you consider increasing the bug severity?

For more information, please visit auto_nag documentation.

Comment 13

[Jens Stutte [:jstutte]]

GeckoChildProcessHost::AsyncLaunch has been largely reworked in the meantime. Which does not mean it cannot be improved, but I think this specific bug is obsolete.

Comment 14

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

From the original description of this bug, this is more of a UI problem: when the process launch fails, the user gets a blank page, instead of retrying or showing an error. So, I don't know if the internal rearchitecting of GeckoChildProcessHost is relevant here.

Comment 15

[Jens Stutte [:jstutte]]

(In reply to Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧ from comment #14)

From the original description of this bug, this is more of a UI problem: when the process launch fails, the user gets a blank page, instead of retrying or showing an error. So, I don't know if the internal rearchitecting of GeckoChildProcessHost is relevant here.

OK, fair enough. I'd say, given that we added at least telemetry and log message here, the minimum requirement of giving "some" feedback has been fulfilled and we can keep this defect closed. But I filed bug 1795821 as an enhancement and made it depend on the bugs that were linked here with "see also" only. I hope that reflects the state better.

Comment 16

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

(In reply to Jens Stutte [:jstutte] from comment #15)

But I filed bug 1795821 as an enhancement and made it depend on the bugs that were linked here with "see also" only. I hope that reflects the state better.

That's a good summary of the issues as I understand them; thanks for filing it.