1448322 - consider unifying AllocPFoo(Child|Parent) with RecvPFoo(Child|Parent)Constructor

Status: NEW

(Core :: IPC, enhancement, P3)

Description

[Nathan Froyd [:froydnj]]

Typical generated code for constructing a managed actor looks like:

    // Allocate.
    actor = AllocPCacheStorageParent(aNamespace, aPrincipalInfo);
    if ((!(actor))) {
        NS_WARNING("Error constructing actor PCacheStorageParent");
        return MsgValueError;
    }

    // Bookkeeping.
    (actor)->SetManager(this);
    RegisterID(actor, (handle__).mId);
    (actor)->SetIPCChannel(GetIPCChannel());
    (mManagedPCacheStorageParent).PutEntry(actor);
    (actor)->mState = mozilla::dom::cache::PCacheStorage::__Start;

    // Initialization.
    if ((!(RecvPCacheStorageConstructor(mozilla::Move(actor), mozilla::Move(aNamespace), mozilla::Move(aPrincipalInfo))))) {
        mozilla::ipc::ProtocolErrorBreakpoint("Handler returned error code!");
        // Error handled in mozilla::ipc::IPCResult
        return MsgProcessingError;
    }

AllocPCacheStorageParent and RecvPCacheStorageConstructor are both virtual functions; AllocPCacheStorageParent is a pure virtual function that must be overridden, while RecvPCacheStorageConstructor has a default, return-success implementation.

I don't know if there is a lot of benefit in having two separate virtual functions when all the work could really be accomplished in Alloc.  Recv*Constructor is rarely overridden, and when it is, it seems like work that could easily be done in Alloc.  Maybe we'd want to have a richer set of return values from Alloc, so error reporting could be better...but ditching a whole bunch of virtual functions from vtables and associated calls seems like a pretty reasonable thing to do.

bkelly, dom/ is probably the largest user of this sort of functionality.  Is there anything wrong with the above idea?

Comment 1

[Nathan Froyd [:froydnj]]

I guess one problem might be if initialization of the actor required all the bookkeeping to be set up already?

Comment 2

[Nathan Froyd [:froydnj]]

Another, much hairier alternative, would be to keep the current setup, but require explicit annotations in the .ipdl files that this secondary initialization is required.  At least that way we could remove the vtable entries for the majority of the Recv*Constructor functions, while keeping that functionality available if people really needed it.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Nathan, the main reason I have had to use a separate RecvConstructor in addition to the alloc method is when I want to call `Send__delete__(result)` from the constructor when an error condition is detected.  Currently this cannot be done safely from the alloc method without triggering a crash, AFAIK.

My gut feeling is that making alloc handle this kind of thing would be more trouble than its worth.  Maybe its easier than I think, though.

Comment 4

[Simon Giesecke [:sg] [he/him]]

(In reply to Nathan Froyd [:froydnj] from comment #2)

Another, much hairier alternative, would be to keep the current setup, but
require explicit annotations in the .ipdl files that this secondary
initialization is required. At least that way we could remove the vtable
entries for the majority of the Recv*Constructor functions, while keeping
that functionality available if people really needed it.

This sounds like a good idea, in particular when involving move-only parameters (which require a const_cast if they are consumed in the Alloc* function), or other complex parameters, which could be moved in the combined function then as well.