Closed Bug 540111 Opened 10 years ago Closed 10 years ago

IPDL: Support multiple protocol managers

Categories

(Core :: IPC, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: mozilla+ben, Assigned: cjones)

References

Details

(Whiteboard: [land m-c])

Attachments

(5 files)

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.
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.
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.
Assignee: nobody → jones.chris.g
Attachment #422793 - Flags: review?(bent.mozilla)
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.
Attachment #422799 - Flags: review?(bnewman)
(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.
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?
(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.
(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 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.
Attachment #422799 - Flags: review?(bnewman) → review+
(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 ;).
Blocks: 541589
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.
Attachment #422793 - Flags: review?(bent.mozilla) → review+
Attachment #422796 - Flags: review?(bent.mozilla) → review+
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!
Attachment #422801 - Flags: review?(bent.mozilla) → review+
You need to log in before you can comment on or make changes to this bug.