Closed Bug 1696460 Opened 4 years ago Closed 4 years ago

Aggressively terminate content processes when IChildProcess.start fails

Categories

(GeckoView :: Sandboxing, defect, P1)

Unspecified
Android

Tracking

(firefox88 fixed)

RESOLVED FIXED
88 Branch
Tracking Status
firefox88 --- fixed

People

(Reporter: bugzilla, Assigned: agi)

References

Details

(Whiteboard: [geckoview:m88])

Attachments

(3 files, 1 obsolete file)

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.

Severity: -- → S3
Priority: -- → P1
Whiteboard: [geckoview:m88]
Rank: 5

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).
Assignee: nobody → agi
Status: NEW → ASSIGNED

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).

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.

Blocks: 1697503
Pushed by asferro@mozilla.com: 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

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 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.

Attachment #9238699 - Attachment is obsolete: true

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

Component: General → Sandboxing
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: