961313 - Add transport indicator to TURN logs

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect)

Description

[Nils Ohlmeier [:drno]]

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.

Comment 1

[Byron Campen [:bwc]]

Attachment: Add the transport protocol to candidate labels.

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.

Comment 2

[Byron Campen [:bwc]]

https://tbpl.mozilla.org/?tree=Try&rev=5a151e92130b

Comment 3

[Adam Roach [:abr]]

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.

Comment 4

[Byron Campen [:bwc]]

Attachment: Add the transport protocol to candidate labels.

Nit fix.

Comment 5

[Byron Campen [:bwc]]

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

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0b2f7df3aada

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Backed out for buffered_stun_socket_unittest crashes.
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb68c3689b36

https://tbpl.mozilla.org/php/getParsedLog.php?id=33463800&tree=Mozilla-Inbound

Comment 8

[Byron Campen [:bwc]]

Now how did this pass try I wonder...

Comment 9

[Byron Campen [:bwc]]

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

Comment 10

[Byron Campen [:bwc]]

Needinfoing to remind myself to check back on some rebuilds on my try push.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ce887b471785

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Backed out for the same crashes as last time.
https://hg.mozilla.org/integration/mozilla-inbound/rev/02118ca19721

https://tbpl.mozilla.org/php/getParsedLog.php?id=33627983&tree=Mozilla-Inbound

Comment 13

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 14

[Byron Campen [:bwc]]

It looks like either bzexport crapped out without me noticing, or I just forgot to attach the new patch. Give me a sec.

Comment 15

[Byron Campen [:bwc]]

Attachment: Add the transport protocol to candidate labels.

Fix printf width bug that causes a crash.

Comment 16

[Byron Campen [:bwc]]

Comment on attachment 8365990 [details] [diff] [review]
Add the transport protocol to candidate labels.

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

Carry forward r+.

Comment 17

[Byron Campen [:bwc]]

https://tbpl.mozilla.org/?tree=Try&rev=9f0ad18b112d

Comment 18

[Byron Campen [:bwc]]

Looks like we're tripping over bugs elsewhere in the code. Trying to fix.

Comment 19

[Byron Campen [:bwc]]

Attachment: Add the transport protocol to candidate labels.

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.

Comment 20

[Byron Campen [:bwc]]

https://tbpl.mozilla.org/?tree=Try&rev=d6680e7ff869

Comment 21

[Byron Campen [:bwc]]

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.

Comment 22

[Byron Campen [:bwc]]

https://tbpl.mozilla.org/?tree=Try&rev=46114e237aa5

Comment 23

[Adam Roach [:abr]]

Comment on attachment 8366103 [details] [diff] [review]
Add the transport protocol to candidate labels.

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

lgtm

Comment 24

[Byron Campen [:bwc]]

Pushing to try again, since the previous one must have generated some broken stuff.

https://tbpl.mozilla.org/?tree=Try&rev=440b0ebc10e3

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ef6c3c3e11b1

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ef6c3c3e11b1