Aggressively terminate content processes when IChildProcess.start fails
Categories
(GeckoView :: Sandboxing, defect, P1)
Tracking
(firefox88 fixed)
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.
Updated•4 years ago
|
Updated•4 years ago
|
Assignee | ||
Comment 1•4 years ago
|
||
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).
Updated•4 years ago
|
Assignee | ||
Comment 2•4 years ago
|
||
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).
Assignee | ||
Comment 3•4 years ago
|
||
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 5•4 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/bc11e6107e13
https://hg.mozilla.org/mozilla-central/rev/9c4199568bbc
https://hg.mozilla.org/mozilla-central/rev/6dba4650eb52
Assignee | ||
Comment 6•3 years ago
|
||
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•3 years ago
|
||
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•2 years ago
|
||
Moving content process management bugs to the new GeckoView::Sandboxing component.
Updated•1 year ago
|
Description
•