Closed Bug 1488993 Opened 2 years ago Closed 1 year ago

Fix PreallocatedProcessManager “blocker” logic around preallocated processes and errors

Categories

(Core :: DOM: Content Processes, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla65
Tracking Status
firefox64 --- fixed
firefox65 --- fixed

People

(Reporter: jld, Assigned: jld)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
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 on attachment 9008597 [details]
Bug 1488993 - Fix PreallocatedProcessManager blocker handling

Mike Conley (:mconley) (:⚙️) has approved the revision.
Attachment #9008597 - Flags: review+
Attachment #9008597 - Attachment description: Bug 1488993 - Fix PreallocatedProcessManager "blocker" handling → Bug 1488993 - Fix PreallocatedProcessManager blocker handling
Pushed by jedavis@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/933bd1aed20f
Fix PreallocatedProcessManager blocker handling r=mconley
https://hg.mozilla.org/mozilla-central/rev/933bd1aed20f
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
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).
(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.
Backed out: https://hg.mozilla.org/mozilla-central/rev/b86d2f48f89c72eaeba103d4d413530c9686e448
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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).
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.
Attachment #9008597 - Attachment is obsolete: true
Attachment #9017745 - Attachment description: Bug 1488993 - Fix PreallocatedProcessManager blocker management (v2) → Bug 1488993 - Fix PreallocatedProcessManager blocker management (v2).
Pushed by jedavis@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cea43bc88c7a
Fix PreallocatedProcessManager blocker management (v2). r=mconley,smaug
Pushed by jedavis@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e0e1a14b592f
Fix PreallocatedProcessManager blocker management (v2). r=mconley,smaug
https://hg.mozilla.org/mozilla-central/rev/e0e1a14b592f
Status: REOPENED → RESOLVED
Closed: 1 year ago1 year ago
Resolution: --- → FIXED
Target Milestone: mozilla64 → mozilla65
You need to log in before you can comment on or make changes to this bug.