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)

defect

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)

      No description provided.
Attachment #702815 - Flags: review?(adam)
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.
Priority: -- → P2
Whiteboard: [webrtc][blocking-webrtc+]
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.
I'm not loving it, but I think it's probably OK.... We could certainly add some sort of state check to suppress it.
(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.
Whiteboard: [webrtc][blocking-webrtc+] → [webrtc][blocking-webrtc+][nICEr][nICEr-upstream-needed]
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.
Blocks: 833557
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)
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
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.
Attachment #709506 - Attachment is obsolete: true
Attachment #709511 - Flags: review?(adam)
Blocks: 796463
Attachment #702815 - Attachment is obsolete: true
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+
https://hg.mozilla.org/mozilla-central/rev/f0422702a077
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
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.