Closed Bug 1285364 Opened 4 years ago Closed 4 years ago

Add a callback for top-level actors to self-destruct

Categories

(Core :: IPC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: dvander, Assigned: dvander)

References

Details

Attachments

(1 file, 1 obsolete file)

A pattern that comes up quite a bit is a top-level actor holding a reference to itself, then posting a release-ref to the event loop in ActorDestroy.

We can simplify this a bit by having a DeallocPBlah callback that fires after DeallocShmems.

This is not good enough to eliminate the PostTask to release the final ref. MessageChannel can have outstanding tasks and making those cancelable/revocable looks like a fair bit of work. We also can't post DeallocPBlah from IPDL since this would break older protocols.

But this still provides a cleaner option than before, and is symmetrical with how sub-actors are destroyed.
Attached patch patch (obsolete) — Splinter Review
PGPUParent.h sample generated code:

    virtual void
    DeallocPGPUParent();

PGPUParent.cpp sample generated code:

auto PGPUParent::OnChannelClose() -> void
{
    DestroySubtree(NormalShutdown);
    DeallocSubtree();
    DeallocShmems();
    DeallocPGPUParent();
}

auto PGPUParent::OnChannelError() -> void
{
    DestroySubtree(AbnormalShutdown);
    DeallocSubtree();
    DeallocShmems();
    DeallocPGPUParent();
}

auto PGPUParent::DeallocPGPUParent() -> void
{
}
Attachment #8768934 - Flags: review?(wmccloskey)
> We also can't post DeallocPBlah from IPDL since this would break older protocols.

Why? This is an entirely new method. Couldn't people who already do the posting opt into it?

Generally I think that using revocable tasks is better, but posting DeallocPBlah could be an intermediate step to that.
(In reply to Bill McCloskey (:billm) from comment #2)
> > We also can't post DeallocPBlah from IPDL since this would break older protocols.
> 
> Why? This is an entirely new method. Couldn't people who already do the
> posting opt into it?
> 
> Generally I think that using revocable tasks is better, but posting
> DeallocPBlah could be an intermediate step to that.

Even though it's unused, if we post it to the event loop and an actor enqueues a self-destruct task in ActorDestroy, we'll potentially have a use-after-free.
Ah, I see. But if we post it before DestroySubtree, then we should be okay, right?

I guess then maybe there will be issues if an actor wants to use this but also wants to post some other runnable in ActorDestroy that should happen first. That seems a bit weird though.
Attached patch patch v2Splinter Review
Fixes use-after-free. This looks like it works, starting a try run though.
Attachment #8768934 - Attachment is obsolete: true
Attachment #8768934 - Flags: review?(wmccloskey)
Attachment #8769351 - Flags: review?(wmccloskey)
Attachment #8769351 - Flags: review?(wmccloskey) → review+
Pushed by danderson@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/375d352b0741
Add a callback for top-level actors to free themselves. (bug 1285364, r=billm)
https://hg.mozilla.org/mozilla-central/rev/375d352b0741
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.