Provide a reference counted wrapper for PBackground handles

NEW
Unassigned

Status

()

Core
IPC
3 years ago
3 years ago

People

(Reporter: blassey, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Right now we return a raw pointers (for example https://dxr.mozilla.org/mozilla-central/source/ipc/glue/BackgroundChild.h#59). I think ideally we'd like to reference count and handle the shutdown when the reference count goes to 0.

Comment 1

3 years ago
Why? Aren't channels 1:1 owned by their toplevel actor?
Do you mean for actors? Some are already reference counted. You can find examples by searching the code for AddIPDLReference(). There's been some discussion of doing that in a more automated way, but I can't find the bug.

Automating shutdown is difficult. The user of the actor needs to be aware that shutdown is happening or else things get really racy. For example, if the parent refcount drops to 0 and we destroy the actor, what happens if the child still has a reference and sends a message to the now dead actor?

Right now, when actors are reference counted, IPDL keeps its own reference. That way the object won't be freed until we've explicitly signaled that no IPC will happen for the object anymore. At that point we free the actor when all the other references are dropped. So the refcounting is only useful for keeping the object alive beyond the time when IPC is finished.

There's definitely room to improve how this stuff works, though. For example, we could keep the actor alive until both sides have agreed, via some protocol, not to use the object anymore. Were there any bugs that motivated this report? We've been discussing this for a while, but any new examples would help.
Also, top-level actors are a different story. Shutting them down often requires shutting down a separate thread (compositor, hang detection, GMP).
I think there was some confusion due to the title of this bug, but empirically it seems GetForCurrentThread()/GetOrCreateForCurrentThread() need to be paired with CloseForCurrentThread() or else you'll hit an assertion on shutdown (related to TLS not being cleaned up).

So the question is if making those return RefPtrs with a custom cleanup part could automate that.
Summary: Provide a reference counted wrapper for IPC channel handles → Provide a reference counted wrapper for PBackground handles
(In reply to Gian-Carlo Pascutto [:gcp] from comment #4)
> I think there was some confusion due to the title of this bug, but
> empirically it seems GetForCurrentThread()/GetOrCreateForCurrentThread()
> need to be paired with CloseForCurrentThread() or else you'll hit an
> assertion on shutdown (related to TLS not being cleaned up).

No, you shouldn't have to call CloseForCurrentThread ever. You *can* call it if that makes sense (we need to do that for workers, for example, because we reuse threads and don't want one worker's messages to leak to another), but in general you don't need to.

What's the assertion you're talking about?
https://dxr.mozilla.org/mozilla-central/source/ipc/glue/BackgroundImpl.cpp#436

This gets hit when the Media code stops the webrtc threads (which are the ones to which PBackgroundChild is attached) in its own shutdown handler.

It looks like (PBackground)ChildImpl assumes it will get to clean that stuff up itself in it's own shutdown observer, but that fails because the above gets called first.
Oh, right... I forgot that we changed this. The issue is that our threadlocal destructor runs after the chromium stuff has already been torn down, so it can't be done automatically... Sorry about that!

This really shouldn't be reference-counted though. You need to determine exactly when you want to shut down your thread, and call BackgroundChild::CloseForCurrentThread() sometime before. Exactly when will depend on what your thread is designed for I guess...
You need to log in before you can comment on or make changes to this bug.