Closed Bug 961313 Opened 6 years ago Closed 6 years ago

Add transport indicator to TURN logs

Categories

(Core :: WebRTC: Networking, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: drno, Assigned: bwc)

Details

Attachments

(1 file, 3 obsolete files)

Right now the debug output from nICEr for TURN contains no indication if messages were send or received via UDP or TCP. This makes it very hard to debug network problems without an additional network trace.

Please add indication to the nICEr log messages if TURN messages were send or received via UDP or TCP.
Assignee: nobody → docfaraday
This should fix the problem, and numerous similar problems. It also fixes a bug where the label (and codeword) for a candidate will be the same regardless of transport protocol, which can lead to duplicates very easily.
Attachment #8363940 - Flags: review?(adam)
Comment on attachment 8363940 [details] [diff] [review]
Add the transport protocol to candidate labels.

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

lgtm. Minor style nit inline.

::: media/mtransport/third_party/nICEr/src/net/transport_addr.c
@@ +58,5 @@
>      /* Max length for normalized IPv6 address string represntation is 39 */
>      char buffer[40];
> +    const char *protocol;
> +
> +    switch (addr->protocol) {

Local style appears to omit the space before the brace.
Attachment #8363940 - Flags: review?(adam) → review+
Nit fix.
Attachment #8363940 - Attachment is obsolete: true
Comment on attachment 8364056 [details] [diff] [review]
Add the transport protocol to candidate labels.

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

Carry forward r=abr
Attachment #8364056 - Flags: review+
Keywords: checkin-needed
Now how did this pass try I wonder...
Looks like I need to be running talos. My bad. I think I know the cause, trying a patch now.

https://tbpl.mozilla.org/?tree=Try&rev=b9b482c6c090
Needinfoing to remind myself to check back on some rebuilds on my try push.
Flags: needinfo?(docfaraday)
Flags: needinfo?(docfaraday)
Keywords: checkin-needed
FWIW, there's not an easy way to run cpp unit tests on Try at the moment (-u all is the only way I know of at the moment), but given that it crashes on all platforms, I would think you could try to reproduce this locally too.
It looks like either bzexport crapped out without me noticing, or I just forgot to attach the new patch. Give me a sec.
Fix printf width bug that causes a crash.
Attachment #8364056 - Attachment is obsolete: true
Comment on attachment 8365990 [details] [diff] [review]
Add the transport protocol to candidate labels.

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

Carry forward r+.
Attachment #8365990 - Flags: review+
Looks like we're tripping over bugs elsewhere in the code. Trying to fix.
Fix a bug where an IP version was being passed instead of a protocol, and a bug where a failure in nr_transport_addr_fmt_addr_string would cause a null pointer dereference later.
Attachment #8365990 - Attachment is obsolete: true
Comment on attachment 8366103 [details] [diff] [review]
Add the transport protocol to candidate labels.

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

Some fixes that need review.
Attachment #8366103 - Flags: review?(adam)
Comment on attachment 8366103 [details] [diff] [review]
Add the transport protocol to candidate labels.

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

lgtm
Attachment #8366103 - Flags: review?(adam) → review+
Pushing to try again, since the previous one must have generated some broken stuff.

https://tbpl.mozilla.org/?tree=Try&rev=440b0ebc10e3
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ef6c3c3e11b1
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.