521929 - IPDL: Need to specify destination "stack frame" for RPC replies

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

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

After adding RPC race resolution, I didn't think it was possible for the "wrong" reply, i.e. one to the not-most-recent request, to be delivered to an actor.  But because actors blocked on RPC out-calls will wake up when an async message arrives, then "wrong" replies can indeed be received.  For example

  Parent            Child
-------------      --------
 out-call R1==>
                    answer R1
                    <== async A1
 unblock for A1
 out-call R1==>     <== reply R1
 recv R1 reply
 (???)

Currently IPDL doesn't attempt to determine which "stack frame" the R1 reply is destined for.  We can re-use the message fields added for RPC race detection to disambiguate RPC replies.

Fairly low priority because this bug can't arise in any protocols in the wild so far.

Comment 1

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

Because of an unrelated bug in RPCChannel.cpp, send/recv of sync messages is also susceptible to a similar type of bug.  The RPCChannel bug is that actors blocked on sync send are awoken on async in-messages, which shouldn't be happening.

Comment 2

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

(In reply to comment #1)
> Because of an unrelated bug in RPCChannel.cpp, send/recv of sync messages is
> also susceptible to a similar type of bug.  The RPCChannel bug is that actors
> blocked on sync send are awoken on async in-messages, which shouldn't be
> happening.

Fixed as part of bug 538239 groping in the dark.

Comment 3

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

Attachment: Test

Comment 4

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

Attachment: Test (new files added)

Grr.

Comment 5

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

Attachment: Add a "seqno" field to synchronous messages

This doesn't fix the bug, but it makes the test case abort in RPCChannel.cpp (bad seqno) rather than in the IPDL test itself.

Comment 6

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

Attachment: Save racy RPC replies onto a special stack until they're the reply to the right out-call

This makes the bug go away.

Comment 7

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

Comment on attachment 422677 [details] [diff] [review]
Add a "seqno" field to synchronous messages

I'm a little nervous about protecting the seqno inside locks. Can you add some AssertThreadOwns() assertions to the code to make sure we dont use the seqno in an unexpected way?

Comment 8

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

Comment on attachment 422678 [details] [diff] [review]
Save racy RPC replies onto a special stack until they're the reply to the right out-call

I think this looks ok.

Comment 9

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

Comment on attachment 422677 [details] [diff] [review]
Add a "seqno" field to synchronous messages

r=me with threadsafety assertion and doc comments.

Comment 10

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

Comment on attachment 422677 [details] [diff] [review]
Add a "seqno" field to synchronous messages

Marking r+ based on

(In reply to comment #9)
> (From update of attachment 422677 [details] [diff] [review])
> r=me with threadsafety assertion and doc comments.

Comment 11

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

Pushed http://hg.mozilla.org/projects/electrolysis/rev/d05dcf0d5576
Pushed http://hg.mozilla.org/projects/electrolysis/rev/955285b4ad0f
Pushed http://hg.mozilla.org/projects/electrolysis/rev/9e823e204581

I don't think these patches are terribly important for m-c in and of themselves, but it would be nice to keep "safe" IPDL changes like these in sync to reduce backporting burden.

Comment 12

[Benjamin Smedberg]

http://hg.mozilla.org/mozilla-central/rev/ca51ffe72682
http://hg.mozilla.org/mozilla-central/rev/925601df843f
http://hg.mozilla.org/mozilla-central/rev/c1e297cb449e