1874739 - Require all IPDL managers to be refcounted

Status: RESOLVED FIXED

(Core :: IPC, enhancement)

Description

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

This is the first part of making 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, which will be handled in bug 1875528.

This patch makes all IPDL actors which manage other actors reference counted similar to how all toplevel protocols were made reference counted in bug 1824465.

Comment 1

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

Attachment: Bug 1874739 - Part 1: Make PClientHandle refcounted, r=#dom-worker-reviewers!

This is part of removing [ManualDealloc] from all protocols which manage other
protocols.

Comment 2

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

Attachment: Bug 1874739 - Part 2: Make PClientManager refcounted, r=#dom-worker-reviewers!

This is part of removing [ManualDealloc] from all protocols which manage other
protocols.

Depends on D198611

Comment 3

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

Attachment: Bug 1874739 - Part 3: Make PClientSource refcounted, r=#dom-worker-reviewers!

This is part of removing [ManualDealloc] from all protocols which manage other
protocols.

Depends on D198612

Comment 4

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

Attachment: Bug 1874739 - Part 4: Make PBackgroundIDBDatabase refcounted, r=#dom-storage-reviewers!

This is part of removing [ManualDealloc] from all protocols which manage other
protocols.

Depends on D198613

Comment 5

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

Attachment: Bug 1874739 - Part 5: Make PBackgroundSDBConnection refcounted, r=#dom-storage-reviewers!

This is part of removing [ManualDealloc] from all protocols which manage other
protocols.

Depends on D198614

Comment 6

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

Attachment: Bug 1874739 - Part 6: Make PBackgroundLSDatabase refcounted, r=#dom-storage-reviewers!

This is part of removing [ManualDealloc] from all protocols which manage other
protocols.

Depends on D198615

Comment 7

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

Attachment: Bug 1874739 - Part 7: Make PRemoteWorkerController refcounted, r=#dom-worker-reviewers!

This is part of removing [ManualDealloc] from all protocols which manage other
protocols.

Depends on D198616

Comment 8

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

Attachment: Bug 1874739 - Part 8: Make PQuota refcounted, r=#dom-storage-reviewers!

This is part of removing [ManualDealloc] from all protocols which manage other
protocols.

Depends on D198617

Comment 9

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

Attachment: Bug 1874739 - Part 9: Make PSpeechSynthesis refcounted, r=eeejay!

This is part of removing [ManualDealloc] from all protocols which manage other
protocols.

Depends on D198618

Comment 10

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

Attachment: Bug 1874739 - Part 10: Make PTestShell refcounted, r=#ipc-reviewers!

This is part of removing [ManualDealloc] from all protocols which manage other
protocols.

Depends on D198619

Comment 11

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

Attachment: Bug 1874739 - Part 11: Make PWebBrowserPersistDocument refcounted, r=#ipc-reviewers!

This is part of removing [ManualDealloc] from all protocols which manage other
protocols.

Depends on D198620

Comment 12

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

Attachment: Bug 1874739 - Part 12: Require manager protocols to be refcounted, r=#ipc-reviewers!

This is part of removing [ManualDealloc] from all protocols which manage other
protocols.

Depends on D198621

Comment 13

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

Attachment: Bug 1874739 - Part 13: Introduce the IRefCountedProtocol type, r=#ipc-reviewers!

This acts as a new class between IProtocol and IToplevelProtocol which just
introduces the AddRef and Release virtual methods. It's used to allow generic
code to hold a strong reference to an actor which is structurally known to be
reference counted, and generally will have limited use elsewhere.

Depends on D198622

Comment 14

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

Attachment: Bug 1874739 - Part 14a: 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.

Depends on D198623

Comment 15

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

Attachment: Bug 1874739 - Part 14b: 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.

Depends on D198624

Comment 16

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

Attachment: Bug 1874739 - Part 14c: 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.

Depends on D198625

Comment 17

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

Attachment: Bug 1874739 - Part 14d: 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.

Depends on D198626

Comment 18

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

Attachment: Bug 1874739 - Part 14e: 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.

Depends on D198627

Comment 19

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

Attachment: Bug 1874739 - Part 14f: 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.

Depends on D198628

Comment 20

[Phabricator Automation]

Comment on attachment 9372920 [details]
Bug 1874739 - Part 14a: Make IProtocol hold a strong reference to manager, r=#ipc-reviewers!

Revision D198624 was moved to bug 1875528. Setting attachment 9372920 [details] to obsolete.

Comment 21

[Phabricator Automation]

Comment on attachment 9372921 [details]
Bug 1874739 - Part 14b: Break the strong reference cycle between WebGPUChild and CanvasManagerChild, r=#webgpu-reviewers!

Revision D198625 was moved to bug 1875528. Setting attachment 9372921 [details] to obsolete.

Comment 22

[Phabricator Automation]

Comment on attachment 9372922 [details]
Bug 1874739 - Part 14c: Remove unnecessary VsyncChild strong reference in BrowserChild, r=smaug!

Revision D198626 was moved to bug 1875528. Setting attachment 9372922 [details] to obsolete.

Comment 23

[Phabricator Automation]

Comment on attachment 9372923 [details]
Bug 1874739 - Part 14d: Clean up VsyncParent handling in BrowserParent, r=smaug!

Revision D198627 was moved to bug 1875528. Setting attachment 9372923 [details] to obsolete.

Comment 24

[Phabricator Automation]

Comment on attachment 9372924 [details]
Bug 1874739 - Part 14e: Make BackgroundChildImpl threadsafe refcounted, r=#ipc-reviewers!

Revision D198628 was moved to bug 1875528. Setting attachment 9372924 [details] to obsolete.

Comment 25

[Phabricator Automation]

Comment on attachment 9372925 [details]
Bug 1874739 - Part 14f: Allow cycle-collecting the strong IPDL Manager reference, r=mccr8!

Revision D198629 was moved to bug 1875528. Setting attachment 9372925 [details] to obsolete.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/36f29de9ecf3
Part 1: Make PClientHandle refcounted, r=dom-worker-reviewers,janv,asuth
https://hg.mozilla.org/integration/autoland/rev/334e8ca0a368
Part 2: Make PClientManager refcounted, r=dom-worker-reviewers,janv,asuth
https://hg.mozilla.org/integration/autoland/rev/57772a95fa5b
Part 3: Make PClientSource refcounted, r=dom-worker-reviewers,janv,asuth
https://hg.mozilla.org/integration/autoland/rev/37c8fb73ee13
Part 4: Make PBackgroundIDBDatabase refcounted, r=dom-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/62d76dd5e757
Part 5: Make PBackgroundSDBConnection refcounted, r=dom-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/cff97f852d19
Part 6: Make PBackgroundLSDatabase refcounted, r=dom-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/f3d6b62af9bd
Part 7: Make PRemoteWorkerController refcounted, r=dom-worker-reviewers,janv,asuth
https://hg.mozilla.org/integration/autoland/rev/a53be8113fea
Part 8: Make PQuota refcounted, r=dom-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/77f27307d159
Part 9: Make PSpeechSynthesis refcounted, r=eeejay
https://hg.mozilla.org/integration/autoland/rev/6b4673af0430
Part 10: Make PTestShell refcounted, r=ipc-reviewers,mccr8
https://hg.mozilla.org/integration/autoland/rev/667d7698de58
Part 11: Make PWebBrowserPersistDocument refcounted, r=ipc-reviewers,mccr8
https://hg.mozilla.org/integration/autoland/rev/40f6af615793
Part 12: Require manager protocols to be refcounted, r=ipc-reviewers,mccr8
https://hg.mozilla.org/integration/autoland/rev/bde4d78e3c5c
Part 13: Introduce the IRefCountedProtocol type, r=ipc-reviewers,mccr8

Comment 27

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

Attachment: WIP: Bug 1874739 - Part 14: Move ClientManagerChild out of line

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9d924486b422
Part 14: Move ClientManagerChild out of line. CLOSED TREE

Comment 29

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/36f29de9ecf3
https://hg.mozilla.org/mozilla-central/rev/334e8ca0a368
https://hg.mozilla.org/mozilla-central/rev/57772a95fa5b
https://hg.mozilla.org/mozilla-central/rev/37c8fb73ee13
https://hg.mozilla.org/mozilla-central/rev/62d76dd5e757
https://hg.mozilla.org/mozilla-central/rev/cff97f852d19
https://hg.mozilla.org/mozilla-central/rev/f3d6b62af9bd
https://hg.mozilla.org/mozilla-central/rev/a53be8113fea
https://hg.mozilla.org/mozilla-central/rev/77f27307d159
https://hg.mozilla.org/mozilla-central/rev/6b4673af0430
https://hg.mozilla.org/mozilla-central/rev/667d7698de58
https://hg.mozilla.org/mozilla-central/rev/40f6af615793
https://hg.mozilla.org/mozilla-central/rev/bde4d78e3c5c
https://hg.mozilla.org/mozilla-central/rev/9d924486b422