1652437 - What keeps the pointers alive in `ContentParent::GetAll`?

Status: RESOLVED WORKSFORME

(Core :: DOM: Content Processes, task)

Description

[David Teller [:Yoric] - still alive but not very active]

ContentParent::GetAll returns an array of raw pointers. There is no documentation regarding the lifetime of these pointers, so I'm a tad nervous.

Digging with code and documentation, the only relevant comment I found is [the following]((https://searchfox.org/mozilla-central/rev/af5cff556a9482d7aa2267a82f2ccfaf18e797e9/dom/ipc/ContentParent.h#707-709): "If a content process is identified as troubled or dead, it will be

• removed from this list, but will still be in the sContentParents list for
• the GetAll/GetAllEvenIfDead APIs."

So, assuming that this is true, does it mean that we're leaking content parents and that we can update the documentation of GetAll()?

If this is not true, or if this might not remain true forever, shouldn't we (at least) make sure that we return an array of RefPtr<ContentParent> instead of an array of ContentParent*?

Comment 1

[Andrew McCreight [:mccr8]]

ContentParent uses conventional refcounting (NS_IMPL_CYCLE_COLLECTING_ADDREF(ContentParent)) so leaks should be reported, assuming these are ever add ref'ed.

Comment 2

[David Teller [:Yoric] - still alive but not very active]

Leaks are one possible issue, the other one being dangling pointers.

Comment 3

[Andrew McCreight [:mccr8]]

Looking at a few callers, it seems like the idea is that it is used as a stack-only data structure, and you should only call it with things that can't do something like spin the event loop that could cause content parents to be destroyed. But I agree that the return an array of strong references so that the API is less fragile.

GeckoChildProcessHost::GetAll() uses a callback interface which avoids the need for a temporary container. That one holds a lock to prevent mutation of the linked list it is iterating over, so you might hit a dead lock but you at least won't get a UAF.

Comment 4

[Chris Peterson [:cpeterson]]

needinfo'ing kmag to comment and then close this bug.

Comment 5

[Kris Maglione [:kmag]]

Nothing holds them alive. They're guaranteed to be alive when they're returned because they're automatically removed from sContentParents when they're destroyed. And they're guaranteed to stay alive from that point until the caller gives code a chance to run that can release them.

Returning an array of RefPtrs would probably be a better idea, and I'd accept a patch, but with the current usage, the raw pointers are perfectly safe.