540886 - IPDL: Implement a mechanism to force child actors to wait for IPC traffic

Status: RESOLVED FIXED

(Core :: IPC, enhancement)

Description

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

The parent sends an async message called Attention(), that, when received by the child, causes the child to spin its IPC event loop and then wait for new messages, as if expecting a synchronous call to return.

The parent may then send messages to the child with the expectation that the child is doing nothing except handling the parent's messages.  In particular, the child cannot send synchronous messages to the parent, and any asynchronous messages it sends are deferred until later.

After handling each message, the child continues to wait, unless it receives the AtEase() message from its parent, whereupon it resumes normal activity (sending child->parent messages, servicing other event loops, &c.).

It should be an error for the parent to return to its own event loop without sending the AtEase() message.

I'm not committed to any particular terminology, but Attention()/AtEase() seems a bit more accurate than Freeze()/Unfreeze().  If it's possible to support the desired behavior without adding special methods to IPDL protocols, that would be even better.

Comment 1

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

(In reply to comment #0)
> If it's possible to support the
> desired behavior without adding special methods to IPDL protocols, that would
> be even better.

Agreed.  I'm going to add these to all top-level RPC actor parents.

Currently I like BlockChild()/UnblockChild(), but also don't care much about the terminology.

Comment 2

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

Attachment: Refactor "special message" dispatch in AsyncChannel

bent, again r?ing for potential backporting of this code.

Comment 3

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

Attachment: Offer a BlockChild() interface to RPC protocols that allows parents to prevent children from sending messages back of their own volition until the parent calls UnblockChild()

Comment 4

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

Attachment: Test for BlockChild()/UnblockChild()

Not the greatest test ever, but this feature is pretty hard to test thoroughly.

benjamn, please let me if this interface serves your needs.  BTW, I left checking for UnblockChild-before-returning-to-event-loop for followup work.  I realized that I don't know how to check that ;).

Comment 5

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

Comment on attachment 422853 [details] [diff] [review]
Test for BlockChild()/UnblockChild()

Suits my needs, indeed.

Comment 6

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

(In reply to comment #4)
> BTW, I left
> checking for UnblockChild-before-returning-to-event-loop for followup work.  I
> realized that I don't know how to check that ;).

In nsThread.cpp, there's an |AfterProcessNextEvent()| observer notification that might do the trick.

Comment 7

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

Attachment: Offer a BlockChild() interface to RPC protocols that allows parents to prevent children from sending messages back of their own volition until the parent calls UnblockChild(), v1.1

Comment 8

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

Comment on attachment 422906 [details] [diff] [review]
 Offer a BlockChild() interface to RPC protocols that allows parents to prevent children from sending messages back of their own volition until the parent calls UnblockChild(), v1.1

>+RPCChannel::OnSpecialMessage(uint16 id, const Message& msg)
> ...
>+    default:
>+        return AsyncChannel::OnSpecialMessage(id, msg);

Shouldn't we pass through to SyncChannel?

>+    if (!mChild)
>+        NS_RUNTIMEABORT("child tried to block parent");

Just noticed that we never initialize mChild in the AsyncChannel constructor (it's set on Open, it looks like), should probably do that :)

>+    while (1) {
> ...
>+        while (Connected() && mPending.empty() && mBlockedOnParent) {

I think you can make things much clearer with only a single while loop.

  while (mBlockedOnParent) {
    WaitForNotify();
    if (!Connected()) { ... }
    if (!mPending.empty()) { }
    }
  }

Comment 9

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

(In reply to comment #8)
> (From update of attachment 422906 [details] [diff] [review])
> >+RPCChannel::OnSpecialMessage(uint16 id, const Message& msg)
> > ...
> >+    default:
> >+        return AsyncChannel::OnSpecialMessage(id, msg);
> 
> Shouldn't we pass through to SyncChannel?
> 

SyncChannel doesn't have any special messages it cares about (yet).  Could add a placeholder I guess.

Comment 10

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

Attachment: WIP patch to add a RequestRunToCompletion method to ContentProcessParent.

This interface would be great if it worked.  Every CPOW operation could request run-to-completion from the ContentProcess (only the first request would block the child), and the child would be automatically unblocked when the parent process returned to its event loop.

The problem: calling alert() (for example) in the parent process spins the parent event loop, unblocking the child before the parent has completed the current sequence of CPOW operations.

Is there any way to avoid calling UnblockChild when AfterProcessNextEvent results from an alert()?

Comment 11

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

We could lift the requirement that UnblockChild() be called before returning to the event loop.

Comment 12

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

Attachment: Manually counting event nesting depth with respect to RequestRunToCompletion.

This works.

Comment 13

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

Comment on attachment 423476 [details] [diff] [review]
Manually counting event nesting depth with respect to RequestRunToCompletion.

See followup bug 542341.

Comment 14

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

Pushed http://hg.mozilla.org/projects/electrolysis/rev/81f601eb0c45
Pushed http://hg.mozilla.org/projects/electrolysis/rev/2aa7d72c7b90
Pushed http://hg.mozilla.org/projects/electrolysis/rev/680d4df8c573

Comment 15

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

(In reply to comment #8)
> (From update of attachment 422906 [details] [diff] [review])
> >+    if (!mChild)
> >+        NS_RUNTIMEABORT("child tried to block parent");
> 
> Just noticed that we never initialize mChild in the AsyncChannel constructor
> (it's set on Open, it looks like), should probably do that :)
> 
> >+    while (1) {
> > ...
> >+        while (Connected() && mPending.empty() && mBlockedOnParent) {
> 
> I think you can make things much clearer with only a single while loop.
> 
>   while (mBlockedOnParent) {
>     WaitForNotify();
>     if (!Connected()) { ... }
>     if (!mPending.empty()) { }
>     }
>   }

(In reply to comment #8)
> (From update of attachment 422906 [details] [diff] [review])
> I think you can make things much clearer with only a single while loop.
> 
>   while (mBlockedOnParent) {
>     WaitForNotify();
>     if (!Connected()) { ... }
>     if (!mPending.empty()) { }
>     }
>   }

Shite!  I forgot to change these before pushing.  Will push with next cset.

Comment 16

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

Meh, or I can just push now.

http://hg.mozilla.org/projects/electrolysis/rev/6125df90678a

Sorry about that.

Comment 17

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

http://hg.mozilla.org/mozilla-central/rev/07ed72e5400b
http://hg.mozilla.org/mozilla-central/rev/b89339a2523d
http://hg.mozilla.org/mozilla-central/rev/db49481bae2f
http://hg.mozilla.org/mozilla-central/rev/bb789c6c7713