Closed
Bug 1060103
Opened 10 years ago
Closed 10 years ago
Fix trickle ICE support for steeplechase
Categories
(Core :: WebRTC, defect)
Core
WebRTC
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)
17.32 KB,
patch
|
drno
:
review+
lmandel
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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 | ||
Updated•10 years ago
|
Assignee: nobody → drno
Assignee | ||
Comment 1•10 years ago
|
||
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)
Comment 2•10 years ago
|
||
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)
Comment 3•10 years ago
|
||
Wouldn't it generally be better to have each test case have its own room/channel?
Assignee | ||
Comment 5•10 years ago
|
||
(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.
Assignee | ||
Comment 6•10 years ago
|
||
Added bug 1062957 for per chat room implementation.
Comment 7•10 years ago
|
||
This is now broken on Aurora as well.
Assignee | ||
Comment 8•10 years ago
|
||
Initial fix. Debug information to be removed. Try run: https://tbpl.mozilla.org/?tree=Try&rev=1256e38d0d5d
Assignee | ||
Updated•10 years ago
|
status-firefox32:
--- → unaffected
status-firefox34:
--- → affected
status-firefox35:
--- → affected
Assignee | ||
Comment 9•10 years ago
|
||
Try run: https://tbpl.mozilla.org/?tree=Try&rev=a2df7b10ea98
Attachment #8484585 -
Attachment is obsolete: true
Attachment #8484967 -
Flags: review?(ted)
Comment 10•10 years ago
|
||
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+
Assignee | ||
Comment 11•10 years ago
|
||
Updated according to Ted's feedback. Carrying forward r+=ted
Attachment #8484967 -
Attachment is obsolete: true
Attachment #8485674 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 12•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/72e7f5f6873d
Keywords: checkin-needed
Comment 13•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/72e7f5f6873d
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Comment 14•10 years ago
|
||
Any chance of uplifting this to aurora?
Comment 15•10 years ago
|
||
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 16•10 years ago
|
||
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+
Comment 17•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/2e38c8b7b6f5
status-b2g-v2.1:
--- → fixed
status-b2g-v2.2:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•