533002 - Implement IPDL discard

Status: RESOLVED WONTFIX

(Core :: IPC, defect)

Description

[Benjamin Smedberg]

I think we should implement IPDL `discard` semantics, and I've attached a patch to bug 532208 which uses them to make PBrowserStream stateful.

At least for now, I don't think we should allow anything to race with __delete__, so it requires two-phase destruction states, but that's not a big deal.

Comment 1

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

(In reply to comment #0)
> At least for now, I don't think we should allow anything to race with
> __delete__, so it requires two-phase destruction states, but that's not a big
> deal.

Wholeheartedly agreed.  To support this we'd need something like bug 532044.

Comment 2

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

One obstacle to implementing |discard| for sync and rpc messages is what discarding them should entail.  The original proposal on m.d.t.dom was limited to async only, where discard just means not invoking the Recv*() handler.  I think if at all possible, we should *not* invoke Recv*() or Answer*() handlers for these discarded messages.  But, because the other side expects a return message, we probably need to communicate discarded-ness back across.  Initially I see a few ways to do this.

(1) Return a |false| error code back to C++ Send/Recv*() handlers.  This should work just fine on the parent side, but children are currently allowed to assume a |true| error code and thus could ignore discarded-ness and return garbage return-values to callers.  One way to work around this is to make "normal" messages |void Foo()| and  |discard| messages |bool Foo() __attribute__((warn_unused_result))|.

(2) For possibly-|discard|ed sync/rpc messages, emit the method signature
  bool Foo(..., bool* discarded);
This has the advantage of allowing the error check |if (!Send*(...)| to still work, and makes it easy to catch child code that ignores |discarded|.  The downside is that this outparam gives Send/Call methods 4 possible "return statuses", when only three are valid:  true+discard=false, false+discard=x, true+discard=true.

(3) For possibly-|discard|ed sync/rpc messages, emit the method signature
  Status Send/Call*(...) __attribute__((warn_unused_result));
for both parents and children, where |Status| is a C++ class wrapping |enum eStatus { OK, Error, Discard };|.  (|Status| can't be a bare enum because gcc allows coercing enums to bools, which could lead to nasty bugs.)

I kind of prefer (3), but not particularly strongly.

A second obstacle to |discard|ed sync/rpc messages is determining "fallback" values to use for outparams returned to foreign C/C++ code that ends up sending the possibly-discarded message.  This will definitely have to be taken case by case, but from what I've seen of NPAPI+major plugins it usually seems safe to return NP_ERR_NO_ERROR with NULL or 0 outparams.  Some things like NPN_RequestRead() are even easier, as we can just ignore the request.  We might be able to crib some of this from nsNPAPI*.

Comment 3

[Benjamin Smedberg]

Per workweek discussion, we're going to go with a modified version of 3) where all methods (whether or not they are discardable) return an C++ wrapper around an enum which doesn't auto-convert to bool. Then client code will be changed to

if (IPDLOK != SendSomeMessage()) {
  // failure mode here
}

cjones, does that match your recollection of our discussion?

Comment 4

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

Yes.  Also, in addition to { OK, ERROR, DISCARD }, I think we'll need EINTR.  But more on that next week! ;)

Comment 5

[Andrew McCreight [:mccr8]]

It looks like this is related to the protocol state checking, which is long gone.