540111 - IPDL: Support multiple protocol managers

Status: RESOLVED FIXED

(Core :: IPC, enhancement)

Description

[Ben Newman (:bnewman) (:benjamn)]

Per IRC discussions with cjones, I propose the following enhancement to
the syntax of IPDL manager declarations:

  protocol PManagedByFooOrBar {
    manager PFoo | PBar;
    ...
  };

For the sake of motivation, imagine that the above declation occurs within
the following protocol hierarchy:

  protocol PManagesFooAndBar {
    manages PFoo;
    manages PBar;
  };

  protocol PFoo {
    manager PManagesFooAndBar;
    manages PManagedByFooOrBar;
    ...
  };

  protocol PBar {
    manager PManagesFooAndBar;
    manages PManagedByFooOrBar;
    ...
  };

Compare the above hierarchy with the most similar one achievable without
multiple managers:

  protocol PManagedByFooOrBar {
    manager PManagesFooAndBar;
    ...
  };

  protocol PManagesFooAndBar {
    manages PFoo;
    manages PBar;
    manages PManagedByFooOrBar;
  };

  protocol PFoo {
    manager PManagesFooAndBar;
    ...
  };

  protocol PBar {
    manager PManagesFooAndBar;
    ...
  };

How much better off would we be if IPDL supported multiple managers?
Consider what happens when PBarParent wants to construct a
PManagedByFooOrBar actor pair.

With multiple managers, the PBarParent simply calls its own
SendPManagedByFooOrBarConstructor method, which may be specialized for the
PBarParent case, as may PBarParent::AllocPManagedByFooOrBar.  On the other
side of the IPC channel, PBarChild::AllocPManagedByFooOrBar and
PBarChild::RecvPManagedByFooOrBarConstructor will then be called, and they
may also differ from PFooChild::AllocPManagedByFooOrBar and
PFooChild::RecvPManagedByFooOrBarConstructor.  Even more importantly, when
the PBarParent is finally destroyed, all the PManagedByFooOrBar actors it
has created will be automatically destroyed as well.

For want of multiple managers, I've been using the following workaround,
which seems much uglier.  I declare a union type

  union PFooOrBar {
    PFoo;
    PBar;
  };

so that, when PBarParent needs to construct a PManagedByFooOrBar, it can
call the following method of its manager, passing a reference to itself as
an argument:

  bool
  PManagesFooAndBarParent::
  SendPManagedByFooOrBarConstructor(const PFooOrBar& fob)

The PFooOrBar parameter is necessary so that appropriate
(PBarChild-specific) action may be taken when

  bool
  PManagesFooAndBarChild::AllocPManagedByFooOrBar(const PFooOrBar&)

and

  bool
  PManagesFooAndBarChild::
  RecvPManagedByFooOrBarConstructor(const PFooOrBar&)

are invoked against the PManagesFooAndBarChild on the other side of the
IPC channel.  This PManagesFooAndBarChild actor would otherwise know
nothing about which of its managed protocols requested the
PManagedByFooOrBar.

With multiple managers, this specialization comes for free.  Without
support for multiple managers, I have to simulate it.

Another indispensable feature of IPDL that must be simulated in the
absence of multiple managers is the automatic destruction of subprotocol
actors.  In my workaround strategy, a PBarParent actor must explicitly
track all the PManagedByFooOrBar actors it has asked its manager to
construct, or else when the PBarParent is destroyed those
PManagedByFooOrBar actors will almost certainly outlive it, a violation of
the conceptual relationship between PBarParent and PManagedByFooOrBar.

I'm open to other workaround strategies, but I can't think of anything
cleaner than implementing the multiple manager syntax directly in IPDL.

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

One complication arose during implementation: the |Manager()| interface.  As spec'd (er, implemented) currently it doesn't make sense in the multi-manager world, so I punted on it for multi-managed protocols.  This required refactoring some other code that depended on Manager().  Note that these changes won't result in any behavioral or interface changes for existing generated C++ code.

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Part 1: Add a new IProtocolManager::RemoveManagee interface to break reliance on the |Manager()| interface

bent, I'm requesting review on all the IPDL bits because I suspect we'll end up needing to merge them into m-c and backport.

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Part 2: Subsume direct calls to manager->Dealloc*() into RemoveManagee()

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Part 3: IPDL tests of "multi-manager" feature.

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Part 4: IPDL/C++ test of "multi-manager" feature.

benjamn, let me know if this is approximately the interface you expected, and/or there's more you want tested.  The test is pretty slim because multi-manager didn't really add any new C++ code paths in the generated code, it's almost exclusively a front-end feature.

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Part 5: Allow IPDL actors to be managed by one of a set of possible managers

Comment 7

[Ben Newman (:bnewman) (:benjamn)]

(In reply to comment #1)
> One complication arose during implementation: the |Manager()| interface.  As
> spec'd (er, implemented) currently it doesn't make sense in the multi-manager
> world, so I punted on it for multi-managed protocols.  This required
> refactoring some other code that depended on Manager().  Note that these
> changes won't result in any behavioral or interface changes for existing
> generated C++ code.

I'll grant that the existing Manager() interface doesn't work in the presence of multiple managers.  The problem is just the indeterminacy of the return type, though; any instance of a protocol with multiple managers actually has just one of those managers, right?  Would it be feasible, then, to have multiple overloaded

  virtual bool Manager(PManagerN** result);

methods?  I suppose we could instead have Manager() return a union type over the possible manager types, but that seems like overkill.

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

The least-gross solution I came up with was

  // [generated by IPDL]
  template<typename T>
  struct ManagerFunctor {
    virtual T operator ()(PManager1*) { }
    virtual T operator ()(PManager2*) { }
    //...
  };

  // [generated by ipdl]
  template<typename T>
  void T WithManager(ManagerFunctor<T>* f)
  {
    // Use some home grown-RTTI (it's there already) to dispatch 
    case PMANAGER1:
      return f->operator()(static_cast<PManager1*>(mManager));
    // ...
  }

But of all the schemes I could think of, it seemed less complicated to just let multi-managed subprotocols roll their own Manager() access until home-grown access gets particular nasty/verbose.

How do you need to get at the manager?

Comment 9

[Ben Newman (:bnewman) (:benjamn)]

(In reply to comment #8)
> How do you need to get at the manager?

To be honest, for my multiply-managed protocol (PContextWrapper, managed by PIFrameEmbedding or PTestShell), I don't.  Is that typical?  Perhaps so, since a protocol whose implementation depends on its manager should probably be split into separate protocols, one for each possible manager.

As long as the Manager() method stays put for singly-managed protocols, I'll be content.  PObjectWrapperChild, managed by PContextWrapperChild, needs to be able to ask its manager for the appropriate JSContext* to use, for example.

The only other reason I've been using Manager() was so that PIFrameEmbeddings and PTestShells could ask their manager (PContentProcess) to construct PContextWrappers, and that indirection will go away when this bug is resolved.

Comment 10

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #9)
> (In reply to comment #8)
> > How do you need to get at the manager?
> 
> To be honest, for my multiply-managed protocol (PContextWrapper, managed by
> PIFrameEmbedding or PTestShell), I don't.  Is that typical?  Perhaps so, since
> a protocol whose implementation depends on its manager should probably be split
> into separate protocols, one for each possible manager.
> 

I don't know if it's typical, but I agree that separate concrete implementations (leaf classes) for each manager is the way to go, when the manager is needed.  I imagine the leaf classes sharing an abstract base (that's a subclass of the actor interface) and not having much leaf-specific code.

> As long as the Manager() method stays put for singly-managed protocols, I'll be
> content.  PObjectWrapperChild, managed by PContextWrapperChild, needs to be
> able to ask its manager for the appropriate JSContext* to use, for example.
> 

Yup, it is.  We rely on it elsewhere.

Comment 11

[Ben Newman (:bnewman) (:benjamn)]

Comment on attachment 422799 [details] [diff] [review]
Part 4: IPDL/C++ test of "multi-manager" feature.

I would still welcome a version of the Manager() protocol for multiply-managed protocols, but the behavior on display in these tests should work for my purposes.

Comment 12

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #8)
> The least-gross solution I came up with was
> 
>   // [generated by ipdl]
>   template<typename T>
>   void T WithManager(ManagerFunctor<T>* f)

Hmm ... on second thought, this could be 

  template<class F, typename R>
  R WithManager(F f)
  {
     // autogenerated dynamic dispatch
     case PMANAGER1:
       return f(static_cast<PManager1*>(mManager));
     // ...
  }

where F might be the struct shown above (sans |virtual|).  This is simpler, gives better type checking of F, and eliminates virtual call crud.

Though I still prefer specializing implementations on manager type ;).

Comment 13

[Ben Newman (:bnewman) (:benjamn)]

By the way, these patches are working great for me (see patch in bug 541589).  No longer crashing on shutdown, and much more elegant code.

Comment 14

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

Comment on attachment 422801 [details] [diff] [review]
Part 5: Allow IPDL actors to be managed by one of a set of possible managers

Looks fine to me!

Comment 15

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Pushed http://hg.mozilla.org/projects/electrolysis/rev/d813355607dd
Pushed http://hg.mozilla.org/projects/electrolysis/rev/fffccafd827f
Pushed http://hg.mozilla.org/projects/electrolysis/rev/de1a20e521f8
Pushed http://hg.mozilla.org/projects/electrolysis/rev/d7a343098058
Pushed http://hg.mozilla.org/projects/electrolysis/rev/3b8310fcc35d

Don't really need these on m-c except to make backporting IPDL changes easier.

Comment 16

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Pushed http://hg.mozilla.org/mozilla-central/rev/31474bd89a11
Pushed http://hg.mozilla.org/mozilla-central/rev/b8fcf1bfa808
Pushed http://hg.mozilla.org/mozilla-central/rev/8ab53a49fadf
Pushed http://hg.mozilla.org/mozilla-central/rev/2353ec631528
Pushed http://hg.mozilla.org/mozilla-central/rev/26ee15150bf7