1125337 - Provide a reference counted wrapper for PBackground handles

Status: NEW

(Core :: IPC, defect)

Description

[Brad Lassey [:blassey] (use needinfo?)]

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

[Benjamin Smedberg]

Why? Aren't channels 1:1 owned by their toplevel actor?

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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.

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Also, top-level actors are a different story. Shutting them down often requires shutting down a separate thread (compositor, hang detection, GMP).

Comment 4

[Gian-Carlo Pascutto [:gcp]]

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.

Comment 5

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(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?

Comment 6

[Gian-Carlo Pascutto [:gcp]]

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.

Comment 7

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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...