Closed Bug 521929 Opened 15 years ago Closed 15 years ago

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

Categories

(Core :: IPC, defect)

Product:
Core
Component:
IPC
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: cjones, Assigned: cjones)

Details

(Whiteboard: [land m-c])

Attachments

(3 files, 1 obsolete file)

Test (new files added)
5.17 KB, patch
Details | Diff | Splinter Review
Add a "seqno" field to synchronous messages
8.48 KB, patch
cjones
: review+
Details | Diff | Splinter Review
Save racy RPC replies onto a special stack until they're the reply to the right out-call
5.45 KB, patch
bent.mozilla
: review+
Details | Diff | Splinter Review
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.
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.
(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.
Attached patch Test (obsolete) — — Splinter Review
Assignee: nobody → jones.chris.g
Status: NEW → ASSIGNED
Grr.
Attachment #422635 - Attachment is obsolete: true
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.
Attachment #422677 - Flags: review?(bent.mozilla)
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 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.
Attachment #422678 - Flags: review?(bent.mozilla) → review+
Comment on attachment 422677 [details] [diff] [review] Add a "seqno" field to synchronous messages r=me with threadsafety assertion and doc comments.
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.
Attachment #422677 - Flags: review?(bent.mozilla) → review+
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.
Whiteboard: [land m-c]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: