1696460 - Aggressively terminate content processes when IChildProcess.start fails

Status: RESOLVED FIXED

(GeckoView :: Sandboxing, defect, P1)

Description

[(No longer employed by Mozilla) Aaron Klotz]

We want to make this future-proof for isolatedProcess, so we should add a method to IChildProcess that makes the bound service self-terminate. Then we should modify GeckoProcessManager.unbind such that, when we're bound, we call that self-termination method before unbinding the interface.

Comment 1

[[ex-Mozilla] Agi Sferro | :agi]

Attachment: Bug 1696460 - Ensure GeckoServiceChildProcess is treated as a bound service.

There is a little Woodoo in this patch but here it is:

• According to the Android docs, services that implement onStartService are
  not considered bound services and need to stop themselves. Since we do want
  our child processes to die after every client unbounds, we shouldn't implement
  it.
• Sometimes processes don't die after |onDestroy| is called, this causes
  processes being reused on some esoteric edge case (multiple runtimes, mostly).
  To avoid these corner cases, we call System.exit in onDestroy. Note: chromium
  does this too.
• We now don't need to kill the process on onBind anymore (this is mostly to
  align with chromium).
• We make sure we don't get created multiple times, as that could cause really
  weird behavior which is hard to debug (especially on users' devices).

Comment 2

[[ex-Mozilla] Agi Sferro | :agi]

Attachment: Bug 1696460 - Allow multiple runtimes to run at the same time.

For tests, we want to be able to run multiple runtimes at the same time (each
runtime gets its own process).

This mostly work, except our process allocator does not currently check that it
owns a certain process and it will happily try to take control of a different
runtime's process (and fail).

To make this work we tag each content process with the parent process' UUID so
that we can check whether the process that we try to create is free or not.

This strategy is pretty rudimental and we can iteratively improve on it if we
decide to use multiple runtimes on actual apps (and not just tests).

Comment 3

[[ex-Mozilla] Agi Sferro | :agi]

Attachment: Bug 1696460 - Improve bind error logging.

This error is sent to the native layer so we never log it (at least locally).

This also adds a "cause" exception to have the full stacktrace.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bc11e6107e13
Ensure GeckoServiceChildProcess is treated as a bound service. r=aklotz
https://hg.mozilla.org/integration/autoland/rev/9c4199568bbc
Allow multiple runtimes to run at the same time. r=aklotz
https://hg.mozilla.org/integration/autoland/rev/6dba4650eb52
Improve bind error logging. r=aklotz

Comment 5

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/bc11e6107e13
https://hg.mozilla.org/mozilla-central/rev/9c4199568bbc
https://hg.mozilla.org/mozilla-central/rev/6dba4650eb52

Comment 6

[[ex-Mozilla] Agi Sferro | :agi]

Attachment: Bug 1696460 - Revert "Bug 1607537: Fix timeouts in ParentCrashTest.crashParent and re-enable the test"

The code in this patch is not needed anymore because now we can handle multiple
in-app runtimes after Bug 1696460.

This reverts commit d49a34c51bc537caffcd559cda07be994105cebb.

Comment 7

[Phabricator Automation]

Comment on attachment 9238699 [details]
Bug 1696460 - Revert "Bug 1607537: Fix timeouts in ParentCrashTest.crashParent and re-enable the test"

Revision D123980 was moved to bug 1676216. Setting attachment 9238699 [details] to obsolete.

Comment 8

[Chris Peterson [:cpeterson]]

Moving content process management bugs to the new GeckoView::Sandboxing component.