1019923 - Data channel test failure logs timeouts after failure

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

[Nils Ohlmeier [:drno]]

The full log here https://tbpl.mozilla.org/php/getParsedLog.php?id=40966662&tree=Mozilla-Inbound
shows that the error case handling is not ideal.
It shows that the data channel for pcLocal is open, but then logs an error about pcLocal not switching to open.
Additionally it logs the failure for pcRemote to open its data channel twice.
And the whole test gets killed by the test framework timeout.

Comment 1

[Nils Ohlmeier [:drno]]

Attachment: bug_1019923_fix_timeouts.patch

Try run: https://tbpl.mozilla.org/?tree=Try&rev=80553a61ce50

Comment 2

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8433853 [details] [diff] [review]
bug_1019923_fix_timeouts.patch

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

r=me with nits.

::: dom/media/tests/mochitest/pc.js
@@ +1085,3 @@
>          onSuccess();
>          return;
>        }

Maybe add an is(peer.dataChannels[0].readyState, "connecting", peer + " dataChannels[0] is in state: 'connecting'"); here?

@@ +1086,5 @@
>          return;
>        }
>  
>        // TODO: drno: convert dataChannels into an object and make
>        //             registerDataChannelOPenEvent a generic function

Spotted unrelated typo: OPen

@@ +1093,5 @@
>        } else {
>          peer.registerDataChannelOpenEvents(dataChannelConnected);
>        }
>  
> +      if (typeof onFailure === "function") {

I would use if (onFailure) here to avoid silently accepting non-functions (that way we hear about it when the code below tries to use it as a function, rather than silently work).

Comment 3

[Nils Ohlmeier [:drno]]

Attachment: bug_1019923_fix_timeouts.patch

Addressed NITs from jib.

Carrying forward r+=jib.

Another try run to be safe, because of the ok(..) I added per jib's comment: https://tbpl.mozilla.org/?tree=Try&rev=1bc833711792

Comment 4

[Nils Ohlmeier [:drno]]

I don't see any new problems.
Let me know if I need to refresh the patch for the checkin.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/516a5bdb4f16

Comment 6

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/516a5bdb4f16

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b954ca743de1