Closed Bug 1035428 Opened 7 years ago Closed 6 years ago

Writable callback for TURN TCP is not rescheduled after writing from queue

Categories

(Core :: WebRTC: Networking, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed
Blocking Flags:

People

(Reporter: bwc, Assigned: bwc)

Details

Attachments

(1 file, 6 obsolete files)

If at any point this function gets R_WOULDBLOCK:

http://dxr.mozilla.org/mozilla-central/source/media/mtransport/third_party/nICEr/src/stun/nr_socket_buffered_stun.c#409

We do not reschedule the writable callback, which means that the queue sits unserviced until we attempt to write some new bytes, here:

http://dxr.mozilla.org/mozilla-central/source/media/mtransport/third_party/nICEr/src/stun/nr_socket_buffered_stun.c#399

If this happens during the setup or connectivity establishment phase with TURN TCP, we end up dead in the water, because there are no retransmissions that will cause nr_socket_buffered_stun_sendto to be called.
Assignee: nobody → docfaraday
Status: NEW → ASSIGNED
Attachment #8451937 - Flags: review?(ekr)
Attachment #8451937 - Flags: review?(ekr) → review?(drno)
Comment on attachment 8451937 [details] [diff] [review]
Re-register writeable callback after partially servicing the send queue.

Review of attachment 8451937 [details] [diff] [review]:
-----------------------------------------------------------------

I think the ABORT() usage is worth fixing.
But in general LGTM.

::: media/mtransport/third_party/nICEr/src/stun/nr_socket_buffered_stun.c
@@ +446,5 @@
> +    // Re-arm
> +    NR_SOCKET fd;
> +
> +    if ((r=nr_socket_getfd(sock->inner, &fd)))
> +      ABORT(r);

Where is this ABORT() going to jump to?
Attachment #8451937 - Flags: review?(drno) → review+
I think it will hop back up to the abort: label and do the right thing, but that's just weird enough that I'll change the code.
Incorporate feedback.
Attachment #8451937 - Attachment is obsolete: true
Incorporate feedback.
Attachment #8481458 - Attachment is obsolete: true
Consolidate some code.
Attachment #8481460 - Attachment is obsolete: true
Actually, I can't consolidate that.
Attachment #8481466 - Attachment is obsolete: true
Attachment #8481478 - Attachment is obsolete: true
Comment on attachment 8481479 [details] [diff] [review]
Re-register writeable callback after partially servicing the send queue.

Review of attachment 8481479 [details] [diff] [review]:
-----------------------------------------------------------------

This changed enough that I'd like another pass.
Attachment #8481479 - Flags: review?(drno)
Comment on attachment 8481479 [details] [diff] [review]
Re-register writeable callback after partially servicing the send queue.

Review of attachment 8481479 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM
Attachment #8481479 - Flags: review?(drno) → review+
Byron  - can we land this?
backlog: --- → webRTC+
Rank: 27
Flags: needinfo?(docfaraday)
Priority: -- → P2
Attachment #8481479 - Attachment is obsolete: true
Comment on attachment 8627721 [details] [diff] [review]
Re-register writeable callback after partially servicing the send queue

Review of attachment 8627721 [details] [diff] [review]:
-----------------------------------------------------------------

Unrotted. Carry forward r=drno.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=202e2ea57397
Attachment #8627721 - Flags: review+
Flags: needinfo?(docfaraday)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e9e66c55eaa5
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.