Data channel test failure logs timeouts after failure

RESOLVED FIXED in Firefox 32, Firefox OS v2.0

Status

()

Core
WebRTC
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: drno, Assigned: drno)

Tracking

Trunk
mozilla33
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox31 wontfix, firefox32 fixed, firefox33 fixed, b2g-v2.0 fixed, b2g-v2.1 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

4 years ago
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.
(Assignee)

Updated

4 years ago
Blocks: 1020024
(Assignee)

Comment 1

4 years ago
Created attachment 8433853 [details] [diff] [review]
bug_1019923_fix_timeouts.patch

Try run: https://tbpl.mozilla.org/?tree=Try&rev=80553a61ce50
Attachment #8433853 - Flags: review?(jib)
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).
Attachment #8433853 - Flags: review?(jib) → review+
(Assignee)

Comment 3

4 years ago
Created attachment 8434383 [details] [diff] [review]
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
Attachment #8433853 - Attachment is obsolete: true
(Assignee)

Comment 4

4 years ago
I don't see any new problems.
Let me know if I need to refresh the patch for the checkin.
Keywords: checkin-needed

Comment 6

4 years ago
https://hg.mozilla.org/mozilla-central/rev/516a5bdb4f16
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
https://hg.mozilla.org/releases/mozilla-aurora/rev/b954ca743de1
status-b2g-v2.0: --- → fixed
status-b2g-v2.1: --- → fixed
status-firefox31: --- → wontfix
status-firefox32: --- → fixed
status-firefox33: --- → fixed
You need to log in before you can comment on or make changes to this bug.