Closed Bug 1060103 Opened 10 years ago Closed 10 years ago

Fix trickle ICE support for steeplechase

Categories

(Core :: WebRTC, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla35
Tracking Status
firefox32 --- unaffected
firefox34 --- fixed
firefox35 --- fixed
b2g-v2.1 --- fixed
b2g-v2.2 --- fixed

People

(Reporter: drno, Assigned: drno)

References

Details

Attachments

(1 file, 2 obsolete files)

The trickle ICE support in mochitest so far does not support steeplechase. This needs to be fixed, by adding some form of event loop in the clients storing messages coming from the signaling server.
Assignee: nobody → drno
So I implemented an endless loop of polling for events to receive the trickle ICE candidates from the other side. This works for one test case.

But it turns out this does not work very well with multiple tests cases, like we would like to exercise with steeplechase, because suddenly the test case itself receives the chat messages which are intended for the surrounding steeplechase framework about test case loading etc.

I need to look into alternatives like:
- use different chat rooms for steeplechase vs. the actual test case
- use a different library then Q, which allows me to cancel the last promise at the end of a test case
- send an end of test case event, so the other side can quit its receive loop and not request another promise

Ted: do you have any other/more ideas on how to approach this?
Flags: needinfo?(ted)
We discussed this on IRC, and my suggestion for now was to have the tests (or the template, really) look for an ice_finished message, set a flag on the test object, and have queueMessage look for that flag to stop waiting for messages. Longer-term we should probably do like you suggest and split the harness message channel out separate from the test message channel.
Flags: needinfo?(ted)
Wouldn't it generally be better to have each test case have its own room/channel?
(In reply to Eric Rescorla (:ekr) from comment #3)
> Wouldn't it generally be better to have each test case have its own
> room/channel?

Yes it would. No doubt.
I'll file a separate bug for that for later and get the simpler solution going right now.
Added bug 1062957 for per chat room implementation.
This is now broken on Aurora as well.
Initial fix. Debug information to be removed.

Try run: https://tbpl.mozilla.org/?tree=Try&rev=1256e38d0d5d
Comment on attachment 8484967 [details] [diff] [review]
bug_1060103_add_trickle_ice_for_steeplechase.patch

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

The only thing I don't like about this patch is the change from using promises (wait_for_message().then(..)) to straight callback-passing, but I guess you're changing the semantics from "wait for a single message" to "call this every time this message is seen", so it makes sense.

::: dom/media/tests/mochitest/templates.js
@@ +23,5 @@
>    if (typeof test._remote_answer !== 'undefined') {
>      dump("ERROR: SDP answer: " + test._remote_answer.sdp.replace(/[\r]/g, ''));
>    }
>  
> +  if ((test.pcLocal) && (typeof test.pcLocal.iceConnectionLog !== 'undefined')) {

These parentheses are extraneous, FWIW.

@@ +80,5 @@
> +        });
> +      }
> +      test.next();
> +    }
> +  ],

You could compress these both into a single 'PC_SETUP_SIGNALING_CLIENT' and then just do something like:
pc = test.pcRemote ? test.pcRemote : test.pcLocal;
pc.storeOrAddIceCandidate(new mozRTCIceCandidate(message.ice_candidate));
Attachment #8484967 - Flags: review?(ted) → review+
Updated according to Ted's feedback.

Carrying forward r+=ted
Attachment #8484967 - Attachment is obsolete: true
Attachment #8485674 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/72e7f5f6873d
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Any chance of uplifting this to aurora?
Comment on attachment 8485674 [details] [diff] [review]
bug_1060103_add_trickle_ice_for_steeplechase.patch

Approval Request Comment
[Feature/regressing bug #]: Multi-machine testing for Web RTC
[User impact if declined]: No user impact, test fix
[Describe test coverage new/current, TBPL]: Enables us to test connectivity of WebRTC between Nightly and Aurora
[Risks and why]: Risk to Firefox. Might adversely impact tbpl tests of WebRTC.
[String/UUID change made/needed]: N/A
Attachment #8485674 - Flags: approval-mozilla-aurora?
Comment on attachment 8485674 [details] [diff] [review]
bug_1060103_add_trickle_ice_for_steeplechase.patch

Test framework fix. Aurora+
Attachment #8485674 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: