Closed Bug 1019923 Opened 6 years ago Closed 6 years ago

Data channel test failure logs timeouts after failure

Categories

(Core :: WebRTC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33
Tracking Status
firefox31 --- wontfix
firefox32 --- fixed
firefox33 --- fixed
b2g-v2.0 --- fixed
b2g-v2.1 --- fixed

People

(Reporter: drno, Assigned: drno)

References

Details

Attachments

(1 file, 1 obsolete file)

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 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+
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
I don't see any new problems.
Let me know if I need to refresh the patch for the checkin.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/516a5bdb4f16
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.