1488993 - Fix PreallocatedProcessManager “blocker” logic around preallocated processes and errors

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, enhancement)

Description

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

I found some issues with preallocated process blockers while working on bug 1446161:

* Preallocated processes themselves don't need to be blockers; PreallocatedProcessManager currently supports at most one process, and if we support multiple concurrent launches in the future, it's not something we'd do accidentally

* RemoveBlocker isn't called in some error cases: if the process fails to launch, or if it's destroyed before reaching RecvFirstIdle.  In that case a blocker would be leaked and we'd never preallocate afterwards.

I have a patch for this.

Comment 1

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

Attachment: Bug 1488993 - Fix PreallocatedProcessManager blocker handling

This makes sure to release blockers (so that content process
preallocation can resume) in error cases, and stops making preallocated
processes themselves blockers, because it's unnecessary (we don't
currently support multiple preallocated processes) and not doing it
means not having to handle those error cases as well.

(Also, in the future we might want to allow the possibility of multiple
concurrent launches if the hardware can support it with acceptable
performance.)

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 9008597 [details]
Bug 1488993 - Fix PreallocatedProcessManager blocker handling

Mike Conley (:mconley) (:⚙️) has approved the revision.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/933bd1aed20f
Fix PreallocatedProcessManager blocker handling r=mconley

Comment 4

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/933bd1aed20f

Comment 5

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

So there are a couple of things wrong with this patch, which I found while investigating bug 1491817:

1. We *do* clean up blockers in ActorDestroy — indirectly, via the ipc:content-shutdown observable.  See https://searchfox.org/mozilla-central/rev/6c82481caa506a240a626bb44a2b8cbe0eedb3a0/dom/ipc/PreallocatedProcessManager.cpp#310

2. Previously, the preallocated process would remain a blocker until it had created its first TabChild and then reached an idle state; with this patch, the next preallocation is allowed as soon as the process is taken.  However, there's at least a 1s delay before it will actually try (parent main thread idle followed by 1s timer), so that may not matter in practice.

I was going to back the patch out while I reconsider it, but there are infra problems (bug 1491948).

Comment 6

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

(In reply to Jed Davis [:jld] (⏰UTC-6) from comment #5)
> 2. Previously, the preallocated process would remain a blocker until it had
> created its first TabChild and then reached an idle state; with this patch,
> the next preallocation is allowed as soon as the process is taken.  However,
> there's at least a 1s delay before it will actually try (parent main thread
> idle followed by 1s timer), so that may not matter in practice.

I profiled on Linux; from the first PBrowser constructor to the following main thread idle was 98ms, of which 45ms was spent in nsLookAndFeel::EnsureInit asking GTK about themes.  So this might not make a difference in practice.

Comment 7

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b86d2f48f89c
Backed out changeset 933bd1aed20f

Comment 8

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

Backed out: https://hg.mozilla.org/mozilla-central/rev/b86d2f48f89c72eaeba103d4d413530c9686e448

Comment 9

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

I tried another approach to this, leaving as much of the existing behavior intact as I could without breaking the future async launch change,by moving the AddBlocker calls to immediately after launching for normal launch, or after PreallocatedProcessManager::Take for preallocated.  (We don't need to have a blocker *while* launching, because this is all on the main thread.)

That almost works, and it almost means that the assertion in RemoveBlocker could stay, but it failed intermittently on Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d0b78ff9026c1ac72cc22ab3eac9f5a09278928a

The failured are because the preallocated process was already registered as a blocker when it was taken.  The only thing I can think of that would cause that would be if a newly launched process is recycled (PreallocatedProcessManager::Provide) and then re-taken, all before RecvFirstIdle.  (If it's possible for a process to be launched and then recycled without ever having a PBrowser created, that makes this more plausible, because in that case the FirstIdle runnable won't have been scheduled yet.)

So I'm leaning back towards just not making preallocated processes blockers at any time (which, given comment #6, doesn't seem likely to change anything).

Comment 10

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

Attachment: Bug 1488993 - Fix PreallocatedProcessManager blocker management (v2).

This fixes/adjusts two things about how content process preallocation is blocked:

1. Processes aren't registered as blockers until after they launch
successfully.  The main goal is to not leak a blocker if launch fails,
but we don't need to block *while* launching synchronously, because this
all happens on the main thread.

2. Preallocated processes themselves aren't blockers.  The main goal
here is so that async preallocation doesn't need extra complexity to
avoid being blocked by itself when launch completes.  This mostly
doesn't affect actual behavior, because we currently support at most
one preallocated process.  The difference is the window from when the
process is sent its first PBrowserConstructor until when it's next idle,
where there is now no longer a blocker, but this seems to be relatively
short (~100ms) and we don't even try to launch a new process until at
least 1s + an idle runnable.

This patch does not explicitly RemoveBlocker in ActorDestroy like the
first attempt did, because it's unnecessary: this is handled in the
ipc:content-shutdown observer.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cea43bc88c7a
Fix PreallocatedProcessManager blocker management (v2). r=mconley,smaug

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e0e1a14b592f
Fix PreallocatedProcessManager blocker management (v2). r=mconley,smaug

Comment 13

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/e0e1a14b592f