Closed Bug 1060102 Opened 10 years ago Closed 10 years ago

Write a test case which does NOT use trickle ICE

Categories

(Core :: WebRTC, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla35
Tracking Status
firefox34 --- fixed
firefox35 --- fixed

People

(Reporter: drno, Assigned: drno)

References

Details

Attachments

(1 file, 4 obsolete files)

We should add a mochitest which waits for trickle ICE to over, before sending the SDP over to the other side, testing if we can still interop with devices which don't support trickle ICE (e.g. PSTN gateways).
Assignee: nobody → drno
Depends on: 1041832
Testing all three possible combinations.

Try run: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=fbe8363b3865
Attachment #8498478 - Flags: review?(martin.thomson)
Attachment #8498478 - Flags: review?(docfaraday)
Comment on attachment 8498478 [details] [diff] [review]
bug_1060102_test_non_trickle.patch

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

::: dom/media/tests/mochitest/pc.js
@@ +2081,5 @@
>     *        A PeerConnectionTest object to which the ice candidates gets
>     *        forwarded.
>     */
> +  setupIceCandidateHandler : function
> +    PCW_setupIceCandidateHandler(candidateHandler, endHandler, candidateCallback) {

I'm not seeing what uses candidateCallback

@@ +2092,5 @@
>        info(self.label + ": received iceCandidateEvent");
>        if (!anEvent.candidate) {
>          info(self.label + ": received end of trickle ICE event");
>          self.endOfTrickleIce = true;
> +        //test.signalEndOfTrickleIce(self.label);

Don't forget to remove these comments.

::: dom/media/tests/mochitest/test_peerConnection_noTrickleAnswer.html
@@ +21,5 @@
> +  runNetworkTest(function (options) {
> +    test = new PeerConnectionTest(options);
> +    test.chain.remove('PC_REMOTE_SETUP_ICE_HANDLER');
> +    test.chain.insertBefore('PC_LOCAL_CREATE_OFFER', [
> +      ['PC_REMOTE_SETUP_NOTRICKLE_ICE_HANDLER',

Do we even need to set up a handler here at all? Could we instead omit this step and set up the handler down in PC_LOCAL_GET_FULL_ANSWER, so we don't need to have this entry call test.next() for another entry?

::: dom/media/tests/mochitest/test_peerConnection_noTrickleOffer.html
@@ +21,5 @@
> +  runNetworkTest(function (options) {
> +    test = new PeerConnectionTest(options);
> +    test.chain.remove('PC_LOCAL_SETUP_ICE_HANDLER');
> +    test.chain.insertBefore('PC_REMOTE_SETUP_ICE_HANDLER', [
> +      ['PC_LOCAL_SETUP_NOTRICKLE_ICE_HANDLER',

Similar question here as above.

::: dom/media/tests/mochitest/test_peerConnection_noTrickleOfferAnswer.html
@@ +21,5 @@
> +  runNetworkTest(function (options) {
> +    test = new PeerConnectionTest(options);
> +    test.chain.remove('PC_LOCAL_SETUP_ICE_HANDLER');
> +    test.chain.insertBefore('PC_REMOTE_SETUP_ICE_HANDLER', [
> +      ['PC_LOCAL_SETUP_NOTRICKLE_ICE_HANDLER',

More of the same.

@@ +38,5 @@
> +      ]
> +    ]);
> +    test.chain.remove('PC_REMOTE_SETUP_ICE_HANDLER');
> +    test.chain.insertBefore('PC_LOCAL_CREATE_OFFER', [
> +      ['PC_REMOTE_SETUP_NOTRICKLE_ICE_HANDLER',

More of the same.
Comment on attachment 8498478 [details] [diff] [review]
bug_1060102_test_non_trickle.patch

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

The test.next() stuff here is hairy.

::: dom/media/tests/mochitest/pc.js
@@ +2092,5 @@
>        info(self.label + ": received iceCandidateEvent");
>        if (!anEvent.candidate) {
>          info(self.label + ": received end of trickle ICE event");
>          self.endOfTrickleIce = true;
> +        //test.signalEndOfTrickleIce(self.label);

Delete.

::: dom/media/tests/mochitest/templates.js
@@ +150,5 @@
> +      test.pcLocal.setupIceCandidateHandler(function (label, candidate) {
> +          test.iceCandidateHandler(label, candidate);
> +        }, function (label) {
> +          test.signalEndOfTrickleIce(label);
> +        });

This same block is repeated throughout.  Given that, why not modify setupIceCandidateHandler to include the following two lines:

 endHandler = endHandler || test.iceCandidateHandler.bind(test);
 candidateCallback = candidateCallback || test.signalEndOfTrickleIce.bind(test);

And avoid all this code duplication.

::: dom/media/tests/mochitest/test_peerConnection_noTrickleAnswer.html
@@ +32,5 @@
> +              info("Looks like we were still waiting for Trickle to finish");
> +              test.next();
> +            }
> +          });
> +          test.next();

You have two calls to test.next() off here.  That's pretty confusing.  Why not just wait for gathering to finish before moving to the next step?

If you have a good reason (gathering being stupidly slow is a valid one), then a comment here (or on the above) is necessary.

@@ +55,5 @@
> +    ]);
> +    test.chain.insertAfter('PC_REMOTE_SANE_LOCAL_SDP', [
> +      ['PC_REMOTE_REQUIRE_SDP_CANDIDATES',
> +        function (test) {
> +          ok(test._remote_answer.sdp.contains("a=candidate"), "offer has ICE candidates")

// TODO: sdp.contains("a=end-of-candidates")

::: dom/media/tests/mochitest/test_peerConnection_noTrickleOffer.html
@@ +29,5 @@
> +            info(label + " ignoring ICE candidate: " + candidate.candidate);
> +          }, function (label) {
> +            if (test.pcLocalWaitingForEndOfTrickleIce) {
> +              info("Looks like we were still waiting for Trickle to finish");
> +              test.next();

Same comment as for others.

But now with more.  There's a lot of code duplication here.

For instance, I would like see test.chain.replace('OLD_NAME', 'NEW_NAME', function() {...}) added for these tests.  That would require the remove to come after the insertBefore, but I'm sure you can manage that.

Then rest of this could probably be factored out into new functions.
Attachment #8498478 - Flags: review?(martin.thomson)
Attachment #8498478 - Flags: review?(docfaraday)
Comment on attachment 8498478 [details] [diff] [review]
bug_1060102_test_non_trickle.patch

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

::: dom/media/tests/mochitest/test_peerConnection_noTrickleAnswer.html
@@ +32,5 @@
> +              info("Looks like we were still waiting for Trickle to finish");
> +              test.next();
> +            }
> +          });
> +          test.next();

The default handler will send the candidates over to the other PeerConnection. So we at least need to remove it.
But the problem here is that we need to setup the callback for end of ICE before calling setLocalDescription in the next step, to cover the case where trickeling takes longer (which seems to be the case on e10s).
In most cases on fast machines the callback is not needed because the candidates, including end of trickle, come in right after the setLocalDescription step is over. But that also means we miss it if we install the callback after calling setLocalDescription.
And if we would setup the callback only in case where trickle is not finished by the time we want to send the SDP over to the other PC, we create a race condition between installing the callback and checking the status of trickle ICE. We had issues with several other callback in the past.
So I prefer to setup callbacks pro-actively to avoid race conditions.

I'll add comments to the code to explain the problem.
Updated according to most of the comments.
Attachment #8498478 - Attachment is obsolete: true
Attachment #8499252 - Flags: review?(martin.thomson)
Attachment #8499252 - Flags: review?(docfaraday)
Comment on attachment 8499252 [details] [diff] [review]
bug_1060102_test_non_trickle.patch

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

Missing file.
Attachment #8499252 - Flags: review?(martin.thomson) → review-
Added missing file
Attachment #8499252 - Attachment is obsolete: true
Attachment #8499252 - Flags: review?(docfaraday)
Attachment #8499256 - Flags: review?(martin.thomson)
Attachment #8499256 - Flags: review?(docfaraday)
Comment on attachment 8499256 [details] [diff] [review]
bug_1060102_test_non_trickle.patch

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

::: dom/media/tests/mochitest/nonTrickleIce.js
@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +function makeOffererNonTrickle(chain) {

The two functions here are basically the same still, right down to the typos in the comments.  This is JavaScript.  There is a way...

@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +function makeOffererNonTrickle(chain) {
> +  chain.replace('PC_LOCAL_SETUP_ICE_HANDLER', [
> +    ['PC_LOCAL_SETUP_NOTRICKLE_ICE_HANDLER',

Nit: I find this syntax peculiar.  I think that I'd prefer it if this were simply:

  chain.replace('PC_LOCAL_SETUP_ICE_HANDLER', 'PC_LOCAL_SETUP_NOTRICKLE_ICE_HANDLER',
                function(test) {

With the extra brackets added in chain.replace.  That would limit replace to a single step, but that's OK.

@@ +13,5 @@
> +            // We ignore ICE candidates because we want to full offer
> +          } , function (label) {
> +            if (test.pcLocalWaitingForEndOfTrickleIce) {
> +              // This callback is needed for slow environments where ICE
> +              // trickeling has not finished before the other side needs the

sp. trickeling

@@ +15,5 @@
> +            if (test.pcLocalWaitingForEndOfTrickleIce) {
> +              // This callback is needed for slow environments where ICE
> +              // trickeling has not finished before the other side needs the
> +              // full SDP. In this case this callback will call the step after
> +              // PC_REMOTE_GET_FULL_OFFER

// In this case, this call to test.next() will complete the PC_REMOTE_GET_FULL_OFFER step (see below).
Attachment #8499256 - Flags: review?(martin.thomson)
(In reply to Martin Thomson [:mt] from comment #8)
> Comment on attachment 8499256 [details] [diff] [review]
> bug_1060102_test_non_trickle.patch
> 
> Review of attachment 8499256 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/tests/mochitest/nonTrickleIce.js
> @@ +1,5 @@
> > +/* This Source Code Form is subject to the terms of the Mozilla Public
> > + * License, v. 2.0. If a copy of the MPL was not distributed with this
> > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> > +
> > +function makeOffererNonTrickle(chain) {
> 
> The two functions here are basically the same still, right down to the typos
> in the comments.  This is JavaScript.  There is a way...

I tried to merge them into one function, but it gets really messy, because you have references to LOCAL and REMOTE in both cases, but different orders. And offer and answer parts are different.
I think the result looks messier and is harder to understand then this code duplication.

> @@ +3,5 @@
> > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> > +
> > +function makeOffererNonTrickle(chain) {
> > +  chain.replace('PC_LOCAL_SETUP_ICE_HANDLER', [
> > +    ['PC_LOCAL_SETUP_NOTRICKLE_ICE_HANDLER',
> 
> Nit: I find this syntax peculiar.  I think that I'd prefer it if this were
> simply:
> 
>   chain.replace('PC_LOCAL_SETUP_ICE_HANDLER',
> 'PC_LOCAL_SETUP_NOTRICKLE_ICE_HANDLER',
>                 function(test) {
> 
> With the extra brackets added in chain.replace.  That would limit replace to
> a single step, but that's OK.

I agree that this double array thing is strange.
But all of the chain functions take an array and not just a single step. So you could replace a single step with a sequence of steps. And for consistency reasons I decided to keep it like this for now.
Comment on attachment 8499256 [details] [diff] [review]
bug_1060102_test_non_trickle.patch

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

::: dom/media/tests/mochitest/nonTrickleIce.js
@@ +36,5 @@
> +        if (test.pcLocal.endOfTrickleIce) {
> +          info("Trickle ICE finished already");
> +          test.next();
> +        } else {
> +          info("Waiting for trickle ICE to finish");

Can't we set up the candidate handler here instead of in some previous step, and avoid the interactions/extra complexity/extra comments? Similar question in the other places.
(In reply to Byron Campen [:bwc] from comment #10)
> Comment on attachment 8499256 [details] [diff] [review]
> bug_1060102_test_non_trickle.patch
> 
> Review of attachment 8499256 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/tests/mochitest/nonTrickleIce.js
> @@ +36,5 @@
> > +        if (test.pcLocal.endOfTrickleIce) {
> > +          info("Trickle ICE finished already");
> > +          test.next();
> > +        } else {
> > +          info("Waiting for trickle ICE to finish");
> 
> Can't we set up the candidate handler here instead of in some previous step,
> and avoid the interactions/extra complexity/extra comments? Similar question
> in the other places.

That would create the race condition I described in c#4 where we would check the status of trickle ICE, if it has not finished set the callback handler. But between the check and installing the callback exactly this event can happen. And then we end up with intermittents.
I prefer some complexity for some test cases over new intermittents (with the variety of speed of our test platforms it is better to play safe).
Attachment #8499256 - Flags: review?(docfaraday) → review+
Fixed remaining issues under e10s.

Carrying forward r+=bwc

Try run: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=38cca7d6c8c8
(Note: this patch is not identical, because it was bit-rotten)
Attachment #8499256 - Attachment is obsolete: true
Attachment #8502134 - Flags: review+
Status: NEW → ASSIGNED
Keywords: checkin-needed
Fixed silly mistake in handling the SDP answer.

Carrying forward r+=bwc
Attachment #8502134 - Attachment is obsolete: true
Attachment #8502732 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6275dc4c7cdf
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Comment on attachment 8502732 [details] [diff] [review]
bug_1060102_test_non_trickle.patch

Approval Request Comment
[Feature/regressing bug #]: This test verifies the fix in bug 1073799 (which fixes a bug introduced in 34 when trickle ICE landed, but is already fixed in 35).
[User impact if declined]: The faulty behavior in 34 will ship into release and FF will not be able to call Asterisk and other gateway solution which don't support trickle ICE).
[Describe test coverage new/current, TBPL]: This test covers an area which is not tested since we introduced full trickle ICE in 34.
[Risks and why]: New test failures on TBPL.
[String/UUID change made/needed]: n/a

Note for the sheriffs: this should only land on Aurora after bug 1073799 has landed on Aurora. If this would land first it would show lots of test failures.
Attachment #8502732 - Flags: approval-mozilla-aurora?
Comment on attachment 8502732 [details] [diff] [review]
bug_1060102_test_non_trickle.patch

Bug 1073799 has already landed on Aurora so it's safe to land this test. Aurora+

Nils - We have not sheriff support this weekend. Can you land this on Aurora yourself?
Attachment #8502732 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: needinfo?(drno)
(In reply to Randell Jesup [:jesup] from comment #20)
> https://hg.mozilla.org/releases/mozilla-aurora/rev/e05a92abe9a8

Thanks a lot Randel!
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.