1875528 - Make actors hold a strong reference to their IPDL manager

Status: RESOLVED FIXED

(Core :: IPC, task)

Description

[Nika Layzell [:nika] (ni? for response)]

This will make accessing Manager() safer after an actor's IPC link has been destroyed, as we can hold a strong reference to the manager until the actor's object is fully destroyed.

This requires making all IPDL actors which manage other actors reference counted, similar to how all toplevel protocols were made reference counted in bug 1824465. This is being handled in bug 1874739.

This bug tracks adding the strong reference, and additional code changes will be required to fix the leaks introduced by strong references to managers, and some cycle-collected actors will need to use manual cc-ing to traverse & clear the Manager() edge when unlinking.

Comment 1

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1875528 - Part 1: Make IProtocol hold a strong reference to manager, r=#ipc-reviewers!

This makes accessing Manager() on an IPDL protocol safer, and replaces the
previous ActorLifecycleProxy reference, which would only keep the manager alive
until IPDL drops its reference.

Unfortunately this introduces some leaks due to reference cycles, which will be
fixed in follow-up parts.

Comment 2

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1875528 - Part 2: Break the strong reference cycle between WebGPUChild and CanvasManagerChild, r=#webgpu-reviewers!

The strong reference from CanvasManagerChild to WebGPUChild was never cleared
when the WebGPUChild dies, meaning that it would form a strong cycle through
the Manager() reference.

Under the new system, the WebGPUChild actor is kept alive by the IPC
connection, and will be torn down when the IPC connection dies.

Comment 3

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1875528 - Part 3: Remove unnecessary VsyncChild strong reference in BrowserChild, r=smaug!

This reference created a cycle after the strong Manager() reference added in an earlier part.

With the reference removed, the VsyncChild will be kept alive via the IPC link
instead, and will be torn down when the IPDL actor is destroyed.

Comment 4

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1875528 - Part 4: Clean up VsyncParent handling in BrowserParent, r=smaug!

Previously there was dead code handling DeallocPvsyncParent which hasn't been
called since PVsync was made refcounted. The new logic handles the reference
within IPC, and delays some initialization to the proper constructor.

Comment 5

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1875528 - Part 5: Make BackgroundChildImpl threadsafe refcounted, r=#ipc-reviewers!

While the BackgroundChildImpl actor is not safe to use from any thread other
than the one it was created on, it holds no important thread-bound members at
destruction time, and does no meaningful work in its destructor.

Actors managed by BackgroundChild are occasionally kept alive longer than the
thread they were bound to, meaning that the new Manager() strong reference
would keep BackgroundChildImpl alive past the death of its thread and IPC link.
This change makes the reference counting threadsafe, so that it's OK to destroy
these final references from another thread.

Comment 6

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1875528 - Part 6: Allow cycle-collecting the strong IPDL Manager reference, r=mccr8!

There are a few IPDL actors which are cycle-collected, including PBrowser,
PContent, and PWindowGlobal.

This patch adds support for these actors to traverse and unlink the new strong
Manager() reference added by IPDL, allowing cycles containing these actors to
be properly unlinked and avoiding leaks.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f0941fdbbabb
Part 1: Make IProtocol hold a strong reference to manager, r=ipc-reviewers,mccr8
https://hg.mozilla.org/integration/autoland/rev/3cef14ed0f25
Part 2: Break the strong reference cycle between WebGPUChild and CanvasManagerChild, r=webgpu-reviewers,nical
https://hg.mozilla.org/integration/autoland/rev/535378dd79b0
Part 3: Remove unnecessary VsyncChild strong reference in BrowserChild, r=smaug
https://hg.mozilla.org/integration/autoland/rev/f3bc6e972f79
Part 4: Clean up VsyncParent handling in BrowserParent, r=smaug
https://hg.mozilla.org/integration/autoland/rev/a5c0564f9761
Part 5: Make BackgroundChildImpl threadsafe refcounted, r=ipc-reviewers,mccr8
https://hg.mozilla.org/integration/autoland/rev/dcbbb7316940
Part 6: Allow cycle-collecting the strong IPDL Manager reference, r=mccr8

Comment 8

[Serban Stanca [:SerbanS]]

Backed out for causing leakcheck failures.

• Backout link
• Push with failures
• Failure Log

Comment 9

[BugBot [:suhaib / :marco/ :calixte]]

There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:nika, could you have a look please?
If you still have some work to do, you can add an action "Plan Changes" in Phabricator.
For more information, please visit BugBot documentation.

Comment 10

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1875528 - Part 7: Expose more BrowserParent members to the CC, r=smaug!

It appears that the Element member may have been creating a reference
cycle passing through the new strong WindowGlobalParent::Manager()
reference.

This patch also removes an unused member from BrowserParent which
otherwise may have needed to be cycle-collected.

Depends on D198629

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/652cf57c384a
Part 1: Make IProtocol hold a strong reference to manager, r=ipc-reviewers,mccr8
https://hg.mozilla.org/integration/autoland/rev/25d1d7b3b88e
Part 2: Break the strong reference cycle between WebGPUChild and CanvasManagerChild, r=webgpu-reviewers,nical
https://hg.mozilla.org/integration/autoland/rev/484eaa0f864e
Part 3: Remove unnecessary VsyncChild strong reference in BrowserChild, r=smaug
https://hg.mozilla.org/integration/autoland/rev/d536e57bf993
Part 4: Clean up VsyncParent handling in BrowserParent, r=smaug
https://hg.mozilla.org/integration/autoland/rev/e013a84d994e
Part 5: Make BackgroundChildImpl threadsafe refcounted, r=ipc-reviewers,mccr8
https://hg.mozilla.org/integration/autoland/rev/7ff4113f8b83
Part 6: Allow cycle-collecting the strong IPDL Manager reference, r=mccr8
https://hg.mozilla.org/integration/autoland/rev/af03c285e0a2
Part 7: Expose more BrowserParent members to the CC, r=smaug

Comment 12

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/652cf57c384a
https://hg.mozilla.org/mozilla-central/rev/25d1d7b3b88e
https://hg.mozilla.org/mozilla-central/rev/484eaa0f864e
https://hg.mozilla.org/mozilla-central/rev/d536e57bf993
https://hg.mozilla.org/mozilla-central/rev/e013a84d994e
https://hg.mozilla.org/mozilla-central/rev/7ff4113f8b83
https://hg.mozilla.org/mozilla-central/rev/af03c285e0a2