Closed
Bug 831290
Opened 11 years ago
Closed 11 years ago
Remove assert for done_cb != 0 in failure cases
Categories
(Core :: WebRTC: Networking, defect, P2)
Core
WebRTC: Networking
Tracking
()
RESOLVED
FIXED
mozilla21
People
(Reporter: ekr, Assigned: ekr)
References
Details
(Whiteboard: [webrtc][blocking-webrtc+][nICEr][nICEr-upstream-needed][qa-])
Attachments
(1 file, 2 obsolete files)
2.12 KB,
patch
|
abr
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #702815 -
Flags: review?(adam)
Assignee | ||
Comment 2•11 years ago
|
||
It turns out that done_cb is called when you have an error, so if you have multiple errors, you can see where this goes.
Updated•11 years ago
|
Priority: -- → P2
Whiteboard: [webrtc][blocking-webrtc+]
Comment 3•11 years ago
|
||
Comment on attachment 702815 [details] [diff] [review] Remove assert for done_cb != 0 in failure cases Review of attachment 702815 [details] [diff] [review]: ----------------------------------------------------------------- ::: media/mtransport/third_party/nICEr/src/ice/ice_peer_ctx.c @@ +463,1 @@ > I'm a little confused about the intended behavior here. With this currently proposed change, the behavior of the ice_completed callback in a multiple-error scenario is potentially different depending on when timers are serviced. For example, if nr_ice_peer_ctx_stream_done is called twice before the timers have any time to be serviced, then ice_completed will be called only once. On the other hand, if we call into nr_ice_peer_ctx_stream_done twice with an intervening pass through the timers to service them, then ice_complete will be called twice. We probably don't want that kind of indeterminate behavior, although I admit that I might be missing some context here that makes it okay.
Assignee | ||
Comment 4•11 years ago
|
||
I'm not loving it, but I think it's probably OK.... We could certainly add some sort of state check to suppress it.
Comment 5•11 years ago
|
||
(In reply to Eric Rescorla (:ekr) from comment #4) > I'm not loving it, but I think it's probably OK.... We could certainly add > some sort of state check to suppress it. Yeah, I think we'd be better off with a flag that blocks the ice_complete callback from firing more than once. We don't want code in the callback to suffer if it makes the same kind of assumption that bit us here (perhaps not as important in the browser code as it is when other apps use the library). Come to think of it, once ice_completed is called, aren't we done with all the callbacks on this context? If that's the case, I think we can simply null out pctx->handler in nr_ice_peer_ctx_fire_done.
Updated•11 years ago
|
Whiteboard: [webrtc][blocking-webrtc+] → [webrtc][blocking-webrtc+][nICEr][nICEr-upstream-needed]
Assignee | ||
Comment 6•11 years ago
|
||
I think there may be a more fundamental logic error here, since when I look at this code, I don't expect it to fire twice. I'm going to spend some time looking to see why it is.
Comment 8•11 years ago
|
||
Comment on attachment 702815 [details] [diff] [review] Remove assert for done_cb != 0 in failure cases I'm clearing the [review?] flag on this for the time being. I think we've concluded that this is indicative of a potentially more damaging race situation, and so I suspect that the ultimate patch will look very different than this one.
Attachment #702815 -
Flags: review?(adam)
Comment 9•11 years ago
|
||
Bumping to critical - this is causing several crashes (asserts) per day on m-i, and one would presume that without the assertion optimized builds will just continue on and do ??? If we can assure the ASSERT case is safe in an opt build, we could consider reducing it to NS_ASSERTION (non-crashing), if this will take a bunch of work to do, and reduce to non-blocking if it doesn't cause mis-behavior to the application's requests.
Severity: normal → critical
Assignee | ||
Comment 10•11 years ago
|
||
rereading the code, I think I know what is going on here. In nr_ice_peer_ctx_stream_done(), we only try to schedule the done_cb if all the streams are either completed or failed. We should only get the all completed once, but because streams have two components, they can report failure twice. I think the fix here, as Adam suggests, is simply to record that you fired done, or perhaps add a more overall state variable.
Assignee | ||
Comment 11•11 years ago
|
||
Assignee | ||
Comment 12•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #709506 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #709511 -
Flags: review?(adam)
Updated•11 years ago
|
Attachment #702815 -
Attachment is obsolete: true
Comment 13•11 years ago
|
||
Comment on attachment 709511 [details] [diff] [review] Avoid double done_cb firing in nICEr Review of attachment 709511 [details] [diff] [review]: ----------------------------------------------------------------- This looks correct to me.
Attachment #709511 -
Flags: review?(adam) → review+
Comment 14•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/f0422702a077
Target Milestone: --- → mozilla21
Comment 15•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f0422702a077
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Updated•11 years ago
|
Whiteboard: [webrtc][blocking-webrtc+][nICEr][nICEr-upstream-needed] → [webrtc][blocking-webrtc+][nICEr][nICEr-upstream-needed][qa-]
You need to log in
before you can comment on or make changes to this bug.
Description
•