549088 - Want ability to delay dispatch of async messages to a toplevel message

Status: RESOLVED INCOMPLETE

(Core :: IPC, defect)

Description

[Benjamin Smedberg]

In async streams the problem I keep hitting is that various async messages are being delivered inside of stack frames I didn't expect, and it's becoming hard to reason about. I've been able to deal with each of these issues as I discover them by posting an event, until this last case: the __delete__ message is being delivered inside of an event that is running code on that object. I suppose I could post a runnable to actually run the C++ destructor, but that sounds very complicated and error-prone.

What I'd like is the ability to delay the delivery of particular IPDL messages until the top-level stack frame, with the knowledge that this may break some in-order delivery guarantees. If you mark all incoming async messages as "deferred", you would only get out-of-order delivery of sync/RPC messages with respect to async messages, which in this case at least is just fine.

I suspect the IPDL state machine should continue to function normally (e.g. state changes are not deferred).

I saw a related bug where you would delay the DeallocPFoo call until the RPC stack unwinds, but this case is different: there is no RPC stack, it's just a delayed task which may make RPC calls, unwinds, and expects `this` to still be valid.

Comment 1

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

(In reply to comment #0)
> the __delete__ message is being
> delivered inside of an event that is running code on that object. I suppose I
> could post a runnable to actually run the C++ destructor, but that sounds very
> complicated and error-prone.
> 

Why?  The current impl is doing that.  It's ugly but not particularly complicated IMHO.

> What I'd like is the ability to delay the delivery of particular IPDL messages
> until the top-level stack frame, with the knowledge that this may break some
> in-order delivery guarantees. If you mark all incoming async messages as
> "deferred", you would only get out-of-order delivery of sync/RPC messages with
> respect to async messages, which in this case at least is just fine.
> 

In the RecvFoo() handler, you can post a task to the event loop, or enqueue the message to be processed in the ExitedCxxStack() callback.  Will both of those not work?

Comment 2

[Benjamin Smedberg]

The current impl is doing what? I can defer the destructor using a special DeallocPBrowserStream implementation which fires an event. Having something which defers the Recv__delete__ is even more complicated because IPDL will call the Dealloc immediately on exit.

ExitedCxxStack doesn't work because there is no RPC stack. Posting a task does work, but you have to hand-roll a bunch of extra code.

Comment 3

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

(In reply to comment #2)
> The current impl is doing what? 

Currently, when the parent decides it wants to destroy a browser stream, it enqueues a task to send __delete__.

> I can defer the destructor using a special
> DeallocPBrowserStream implementation which fires an event. Having something
> which defers the Recv__delete__ is even more complicated because IPDL will call
> the Dealloc immediately on exit.
> 

I don't understand why that's more complicated, but either approach seems viable to me.

> ExitedCxxStack doesn't work because there is no RPC stack. Posting a task does
> work, but you have to hand-roll a bunch of extra code.

ExitedCxxStack applies to all RPCChannel C++ stack frames, including those devoted to async and sync messages.  Posting a task would be less code than using ExitedCxxStack, but it's easier to reason about when ExitedCxxStack will be invoked.

Comment 4

[Benjamin Smedberg]

The child stack frame looks like this:

* run-next-task
* TaskWrapper::Run (queued from a ScopedRunnableMethodFactory)
* BrowserStreamParent::DeliverData
* NPP_URLNotify
* plugin calls any RPC method
* RPCChannel processes incoming async PBrowserStreamParent.__delete__ message

ExitedCxxStack can't work because we need to defer the destruction until after DeliverData is done, and there isn't any RPCChannel code above it on the stack.

And deferring sending __delete__ on the child side doesn't help either.

Comment 5

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

What does __delete__ correspond to in your WIP?  Is it sent after the NPN_DestroyStream()/NPP_DestroyStream() negotiation?  Are those synchronous or asynchronous?

The current code assumes that once NPP_DestroyStream() is sent (and the current RPC stack finishes), that the plugin shouldn't be touching the stream anymore.  So just before sending NPP_DestroyStream(), the browser code enqueues a new task to send __delete__ to the child.  This handles the case of NPP_DestroyStream() racing with anything sent by the child.  Would this approach work for your code?

Comment 6

[Benjamin Smedberg]

Yeah, it's

parent:
  async NPN_DestroyStream;
child:
  async NPP_DestroyStream
parent:
  async AcknowledgeDestroy;
child:
  async __delete__;

What's happening is that we send the AcknowledgeDestroy message in the middle of that "DeliverData" event. It would be hard though perhaps not impossible to change that so that we don't send AcknowledgeDestroy until right at the end of the method.

Perhaps we just need a better way of noticing/debugging situations where you're calling a method on a C++ class which is destroyed mid-call. I'd love a valgrind analysis for that!