Closed Bug 1231975 Opened 9 years ago Closed 8 years ago

Mochitests for NAT scenarios

Categories

(Core :: WebRTC: Networking, defect, P1)

Product:
Core
Component:
WebRTC: Networking
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla49
Tracking Flags:
Tracking Status
firefox45 --- affected
firefox49 --- fixed

People

(Reporter: bwc, Assigned: bwc)

References

Details

Attachments

(7 files)

MozReview Request: Bug 1231975 - Part 1: Basic audio mochitests for NAT scenarios. r=drno
58 bytes, text/x-review-board-request
drno
: review+
Details
MozReview Request: Bug 1231975 - Part 2: Break a reference cycle between NrTcpSocketIpc and TCPSocketChild, in the same manner as the UDP case. r=mcmanus
58 bytes, text/x-review-board-request
mcmanus
: review+
Details
MozReview Request: Bug 1231975 - Part 3: Break a reference cycle between PendingResolution and DNSRequestChild. r=drno
58 bytes, text/x-review-board-request
drno
: review+
Details
MozReview Request: Bug 1231975 - Part 4: Add some logging and simplification in TestNrSocket. r=drno
58 bytes, text/x-review-board-request
drno
: review+
Details
MozReview Request: Bug 1231975 - Part 5: Fix an intermittent failure caused by the NAT simulator erroneously canceling NR_ASYNC_WAIT_READ. r=drno
58 bytes, text/x-review-board-request
drno
: review+
Details
MozReview Request: Bug 1231975 - Part 6: Disable NAT tests on android due to bug 1266217 r=drno
58 bytes, text/x-review-board-request
drno
: review+
Details
MozReview Request: Bug 1231975 - Part 7: Disable trickle for the NAT tests, since trickle delays can cause lower priority pairs to get selected. r=drno
58 bytes, text/x-review-board-request
drno
: review+
Details 
      No description provided.
Depends on: 1231977
Rank: 15
Assignee: nobody → docfaraday
Comment on attachment 8730340 [details]
MozReview Request: Bug 1231975 - Part 1: Basic audio mochitests for NAT scenarios. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39815/diff/1-2/
Comment on attachment 8730340 [details]
MozReview Request: Bug 1231975 - Part 1: Basic audio mochitests for NAT scenarios. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39815/diff/2-3/
Comment on attachment 8730340 [details]
MozReview Request: Bug 1231975 - Part 1: Basic audio mochitests for NAT scenarios. r=drno

https://reviewboard.mozilla.org/r/39815/#review36779

In general it looks good and I like the idea.

But the big problem I see is with the use of SpecialPowers.pushPrefEnv() in each test case. I'm under the impression that calling SpecialPowers.pushPrefEnv() from a test case basically will not update the nICEr registry properly.
I'm not sure I remember all the details, but I think in a good case each of the SpecialPowers.pushPrefEnv() calls simply does nothing. In the worst case I think the first call SpecialPowers.pushPrefEnv() will set the |filtering_type| and |mapping_type|, but then the subsequent tests silently fail to update these values because of the caching nature of the nICEr registry and all of your tests run with the initial value. See https://bugzilla.mozilla.org/show_bug.cgi?id=1180388#c25

::: dom/media/tests/mochitest/test_peerConnection_basicAudioNATRelay.html:15
(Diff revision 3)
> -  runNetworkTest(function (options) {
> +  runNetworkTest( () => {
> +    SpecialPowers.pushPrefEnv(
> +      {
> +        'set': [
> +          ['media.peerconnection.nat_simulator.filtering_type', 'PORT_DEPENDENT'],
> +          ['media.peerconnection.nat_simulator.mapping_type', 'PORT_DEPENDENT']
> +        ]
> +      }, function (options) {

I'm still getting confused by this "feature" of JS, but don't you need to name the input variable like this here:
  runNetworkTest((options) => {
so that you can later pass it into the success callback?
Even if it is not needed I think it makes the code a lot more clear if you did.

It's easy to overlook, because plain mochitest on try doesn't pass in any options. Only steeplechase does.

::: dom/media/tests/mochitest/test_peerConnection_basicAudioNATRelay.html:30
(Diff revision 3)
> +        options.expectedRemoteCandidateType = "relayed";
> -    test = new PeerConnectionTest(options);
> +        test = new PeerConnectionTest(options);
> -    test.setMediaConstraints([{audio: true}], [{audio: true}]);
> +        test.setMediaConstraints([{audio: true}], [{audio: true}]);
> -    test.run();
> +        test.run();
> -  });
> +      })
> +  }, { useIceServer: true });

What is this |useIceServer| doing?

::: dom/media/tests/mochitest/test_peerConnection_basicAudioNATRelayTCP.html:15
(Diff revision 3)
> -    bug: "796892",
> -    title: "Basic audio-only peer connection"
> +    bug: "1231975",
> +    title: "Basic audio-only peer connection with port dependent NAT that blocks TCP"
>    });
>  
>    var test;
> -  runNetworkTest(function (options) {
> +  runNetworkTest( () => {

Same here

::: dom/media/tests/mochitest/test_peerConnection_basicAudioNATRelayTCP.html:31
(Diff revision 3)
> +        options.expectedRemoteCandidateType = "relayed-tcp";
> -    test = new PeerConnectionTest(options);
> +        test = new PeerConnectionTest(options);
> -    test.setMediaConstraints([{audio: true}], [{audio: true}]);
> +        test.setMediaConstraints([{audio: true}], [{audio: true}]);
> -    test.run();
> +        test.run();
> -  });
> +      })
> +  }, { useIceServer: true });

Same here

::: dom/media/tests/mochitest/test_peerConnection_basicAudioNATSrflx.html:15
(Diff revision 3)
> -    bug: "796892",
> -    title: "Basic audio-only peer connection"
> +    bug: "1231975",
> +    title: "Basic audio-only peer connection with endpoint independent NAT"
>    });
>  
>    var test;
> -  runNetworkTest(function (options) {
> +  runNetworkTest( () => {

And here

::: dom/media/tests/mochitest/test_peerConnection_basicAudioNATSrflx.html:30
(Diff revision 3)
> +        options.expectedRemoteCandidateType = "serverreflexive";
> -    test = new PeerConnectionTest(options);
> +        test = new PeerConnectionTest(options);
> -    test.setMediaConstraints([{audio: true}], [{audio: true}]);
> +        test.setMediaConstraints([{audio: true}], [{audio: true}]);
> -    test.run();
> +        test.run();
> -  });
> +      })
> +  }, { useIceServer: true });

And here
Attachment #8730340 - Flags: review?(drno)
https://reviewboard.mozilla.org/r/39815/#review36779

Right, you cannot update the nICEr registry this way, but we aren't trying to do that here. These prefs are queried each time a PC is created, and never go into the nICEr registry.

> I'm still getting confused by this "feature" of JS, but don't you need to name the input variable like this here:
>   runNetworkTest((options) => {
> so that you can later pass it into the success callback?
> Even if it is not needed I think it makes the code a lot more clear if you did.
> 
> It's easy to overlook, because plain mochitest on try doesn't pass in any options. Only steeplechase does.

Right, this is an error on my part.

> What is this |useIceServer| doing?

That's a fixture options object being passed as the second param to runNetworkTest. In this case, it is specifying that this test requires an ICE server to be running.
Comment on attachment 8730340 [details]
MozReview Request: Bug 1231975 - Part 1: Basic audio mochitests for NAT scenarios. r=drno

https://reviewboard.mozilla.org/r/39815/#review36873
Attachment #8730340 - Flags: review+
https://reviewboard.mozilla.org/r/39815/#review36779

Good point. I thought this would go into the registry as so many values do. Never mind then.

> That's a fixture options object being passed as the second param to runNetworkTest. In this case, it is specifying that this test requires an ICE server to be running.

So this code here then has a hard dependency on the code from bug 1231981 landing first? That was not clear to me.
(In reply to Nils Ohlmeier [:drno] from comment #7)
> https://reviewboard.mozilla.org/r/39815/#review36779
> 
> Good point. I thought this would go into the registry as so many values do.
> Never mind then.
> 
> > That's a fixture options object being passed as the second param to runNetworkTest. In this case, it is specifying that this test requires an ICE server to be running.
> 
> So this code here then has a hard dependency on the code from bug 1231981
> landing first? That was not clear to me.

Sorry my intention was to comment and at the same time give you and "fix it then r+". That obviously did not work out as intended.
Comment on attachment 8730340 [details]
MozReview Request: Bug 1231975 - Part 1: Basic audio mochitests for NAT scenarios. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39815/diff/3-4/
Attachment #8730340 - Attachment description: MozReview Request: Bug 1231975: Basic audio mochitests for NAT scenarios. r?drno → MozReview Request: Bug 1231975: Basic audio mochitests for NAT scenarios. r=drno
Depends on: 1231981
Comment on attachment 8730340 [details]
MozReview Request: Bug 1231975 - Part 1: Basic audio mochitests for NAT scenarios. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39815/diff/4-5/
Depends on: 1258836
Comment on attachment 8730340 [details]
MozReview Request: Bug 1231975 - Part 1: Basic audio mochitests for NAT scenarios. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39815/diff/5-6/
Comment on attachment 8730340 [details]
MozReview Request: Bug 1231975 - Part 1: Basic audio mochitests for NAT scenarios. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39815/diff/6-7/
Comment on attachment 8730340 [details]
MozReview Request: Bug 1231975 - Part 1: Basic audio mochitests for NAT scenarios. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39815/diff/7-8/
Comment on attachment 8730340 [details]
MozReview Request: Bug 1231975 - Part 1: Basic audio mochitests for NAT scenarios. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39815/diff/8-9/
Comment on attachment 8730340 [details]
MozReview Request: Bug 1231975 - Part 1: Basic audio mochitests for NAT scenarios. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39815/diff/9-10/
Comment on attachment 8730340 [details]
MozReview Request: Bug 1231975 - Part 1: Basic audio mochitests for NAT scenarios. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39815/diff/10-11/
Comment on attachment 8730340 [details]
MozReview Request: Bug 1231975 - Part 1: Basic audio mochitests for NAT scenarios. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39815/diff/11-12/
Review commit: https://reviewboard.mozilla.org/r/46421/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46421/
Attachment #8730340 - Attachment description: MozReview Request: Bug 1231975: Basic audio mochitests for NAT scenarios. r=drno → MozReview Request: Bug 1231975 - Part 1: Basic audio mochitests for NAT scenarios. r=drno
Attachment #8741361 - Flags: review?(mcmanus)
Attachment #8741362 - Flags: review?(drno)
Attachment #8741363 - Flags: review?(drno)
Comment on attachment 8730340 [details]
MozReview Request: Bug 1231975 - Part 1: Basic audio mochitests for NAT scenarios. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39815/diff/12-13/
Comment on attachment 8741361 [details]
MozReview Request: Bug 1231975 - Part 2: Break a reference cycle between NrTcpSocketIpc and TCPSocketChild, in the same manner as the UDP case. r=mcmanus

https://reviewboard.mozilla.org/r/46421/#review43035
Attachment #8741361 - Flags: review?(mcmanus) → review+
Comment on attachment 8741363 [details]
MozReview Request: Bug 1231975 - Part 4: Add some logging and simplification in TestNrSocket. r=drno

https://reviewboard.mozilla.org/r/46425/#review43097

LGTM

::: media/mtransport/test_nr_socket.cpp:360
(Diff revision 1)
> -      if (ingress_allowed && nat_->refresh_on_ingress_ && port_mapping_used) {
> +      if (ingress_allowed) {
> +        r_log(LOG_GENERIC, LOG_INFO, "TestNrSocket %s received from %s via %s",
> +              internal_socket_->my_addr().as_string,
> +              from->as_string,
> +              port_mapping_used->external_socket_->my_addr().as_string);
> +        if (nat_->refresh_on_ingress_) {

As get_port_mapping() can still return a nullptr I think a MOZ_ASSERT(port_mapping_used) could be helpful.
Attachment #8741363 - Flags: review?(drno) → review+
Attachment #8741362 - Flags: review?(drno) → review+
Comment on attachment 8741362 [details]
MozReview Request: Bug 1231975 - Part 3: Break a reference cycle between PendingResolution and DNSRequestChild. r=drno

https://reviewboard.mozilla.org/r/46423/#review43109
Comment on attachment 8730340 [details]
MozReview Request: Bug 1231975 - Part 1: Basic audio mochitests for NAT scenarios. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39815/diff/13-14/
Comment on attachment 8741361 [details]
MozReview Request: Bug 1231975 - Part 2: Break a reference cycle between NrTcpSocketIpc and TCPSocketChild, in the same manner as the UDP case. r=mcmanus

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46421/diff/1-2/
Comment on attachment 8741362 [details]
MozReview Request: Bug 1231975 - Part 3: Break a reference cycle between PendingResolution and DNSRequestChild. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46423/diff/1-2/
Comment on attachment 8741363 [details]
MozReview Request: Bug 1231975 - Part 4: Add some logging and simplification in TestNrSocket. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46425/diff/1-2/
Attachment #8741517 - Flags: review?(drno) → review+
Comment on attachment 8741517 [details]
MozReview Request: Bug 1231975 - Part 5: Fix an intermittent failure caused by the NAT simulator erroneously canceling NR_ASYNC_WAIT_READ. r=drno

https://reviewboard.mozilla.org/r/46547/#review43173

::: media/mtransport/test_nr_socket.cpp
(Diff revision 1)
> -  // Stop listening on all real sockets; we will start listening again
> -  // if the app starts listening to us again.
> -  cancel_port_mapping_async_wait(NR_ASYNC_WAIT_READ);
> -  internal_socket_->cancel(NR_ASYNC_WAIT_READ);

Out of curiosity: was the assumption wrong that the app will set the read poll flag again or why/how is this causing intermittents?
https://reviewboard.mozilla.org/r/46547/#review43173

> Out of curiosity: was the assumption wrong that the app will set the read poll flag again or why/how is this causing intermittents?

Yes, the assumption was wrong. nICEr only sets up the WAIT_READ when the socket is created, which is very different from how WAIT_WRITE is handled.
Attachment #8743872 - Flags: review?(drno)
Comment on attachment 8743872 [details]
MozReview Request: Bug 1231975 - Part 6: Disable NAT tests on android due to bug 1266217 r=drno

https://reviewboard.mozilla.org/r/48081/#review44909

with regrets
Attachment #8743872 - Flags: review?(drno) → review+
Comment on attachment 8730340 [details]
MozReview Request: Bug 1231975 - Part 1: Basic audio mochitests for NAT scenarios. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39815/diff/14-15/
Comment on attachment 8741361 [details]
MozReview Request: Bug 1231975 - Part 2: Break a reference cycle between NrTcpSocketIpc and TCPSocketChild, in the same manner as the UDP case. r=mcmanus

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46421/diff/2-3/
Comment on attachment 8741362 [details]
MozReview Request: Bug 1231975 - Part 3: Break a reference cycle between PendingResolution and DNSRequestChild. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46423/diff/2-3/
Comment on attachment 8741363 [details]
MozReview Request: Bug 1231975 - Part 4: Add some logging and simplification in TestNrSocket. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46425/diff/2-3/
Comment on attachment 8741517 [details]
MozReview Request: Bug 1231975 - Part 5: Fix an intermittent failure caused by the NAT simulator erroneously canceling NR_ASYNC_WAIT_READ. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46547/diff/1-2/
Comment on attachment 8743872 [details]
MozReview Request: Bug 1231975 - Part 6: Disable NAT tests on android due to bug 1266217 r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48081/diff/1-2/
Comment on attachment 8730340 [details]
MozReview Request: Bug 1231975 - Part 1: Basic audio mochitests for NAT scenarios. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39815/diff/15-16/
Comment on attachment 8741361 [details]
MozReview Request: Bug 1231975 - Part 2: Break a reference cycle between NrTcpSocketIpc and TCPSocketChild, in the same manner as the UDP case. r=mcmanus

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46421/diff/3-4/
Comment on attachment 8741362 [details]
MozReview Request: Bug 1231975 - Part 3: Break a reference cycle between PendingResolution and DNSRequestChild. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46423/diff/3-4/
Comment on attachment 8741363 [details]
MozReview Request: Bug 1231975 - Part 4: Add some logging and simplification in TestNrSocket. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46425/diff/3-4/
Comment on attachment 8741517 [details]
MozReview Request: Bug 1231975 - Part 5: Fix an intermittent failure caused by the NAT simulator erroneously canceling NR_ASYNC_WAIT_READ. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46547/diff/2-3/
Comment on attachment 8743872 [details]
MozReview Request: Bug 1231975 - Part 6: Disable NAT tests on android due to bug 1266217 r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48081/diff/2-3/
Comment on attachment 8730340 [details]
MozReview Request: Bug 1231975 - Part 1: Basic audio mochitests for NAT scenarios. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39815/diff/16-17/
Attachment #8743872 - Attachment description: MozReview Request: Bug 1231975 - Part 6: Disable NAT tests on android due to bug 1266217 → MozReview Request: Bug 1231975 - Part 6: Disable NAT tests on android due to bug 1266217 r=drno
Comment on attachment 8741361 [details]
MozReview Request: Bug 1231975 - Part 2: Break a reference cycle between NrTcpSocketIpc and TCPSocketChild, in the same manner as the UDP case. r=mcmanus

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46421/diff/4-5/
Comment on attachment 8741362 [details]
MozReview Request: Bug 1231975 - Part 3: Break a reference cycle between PendingResolution and DNSRequestChild. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46423/diff/4-5/
Comment on attachment 8741363 [details]
MozReview Request: Bug 1231975 - Part 4: Add some logging and simplification in TestNrSocket. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46425/diff/4-5/
Comment on attachment 8741517 [details]
MozReview Request: Bug 1231975 - Part 5: Fix an intermittent failure caused by the NAT simulator erroneously canceling NR_ASYNC_WAIT_READ. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46547/diff/3-4/
Comment on attachment 8743872 [details]
MozReview Request: Bug 1231975 - Part 6: Disable NAT tests on android due to bug 1266217 r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48081/diff/3-4/
https://hg.mozilla.org/integration/mozilla-inbound/rev/4cff2bed67474c49c44c615ba63f1916753660f1
Bug 1231975 - Part 1: Basic audio mochitests for NAT scenarios. r=drno

https://hg.mozilla.org/integration/mozilla-inbound/rev/8aa793156ad00f73da82abff582fee22264b83f4
Bug 1231975 - Part 2: Break a reference cycle between NrTcpSocketIpc and TCPSocketChild, in the same manner as the UDP case. r=mcmanus

https://hg.mozilla.org/integration/mozilla-inbound/rev/1913c67263eb682ce310ec92b501470a594793c7
Bug 1231975 - Part 3: Break a reference cycle between PendingResolution and DNSRequestChild. r=drno

https://hg.mozilla.org/integration/mozilla-inbound/rev/9d0c273048d6c2bf7510d1e6f71a31341e3511f2
Bug 1231975 - Part 4: Add some logging and simplification in TestNrSocket. r=drno

https://hg.mozilla.org/integration/mozilla-inbound/rev/b21f8efbd341280eacf80bbe5500841641994cb1
Bug 1231975 - Part 5: Fix an intermittent failure caused by the NAT simulator erroneously canceling NR_ASYNC_WAIT_READ. r=drno

https://hg.mozilla.org/integration/mozilla-inbound/rev/aa8850420313f0c513f58be87022a8874aa64bf1
Bug 1231975 - Part 6: Disable NAT tests on android due to bug 1266217 r=drno
backed out for mass mochitest bustage like : https://treeherder.mozilla.org/logviewer.html#?job_id=26551894&repo=mozilla-inbound

 14:21:56     INFO -  MochitestServer : launching [u'/home/worker/workspace/build/tests/bin/xpcshell', '-g', '/home/worker/workspace/build/application/firefox', '-v', '170', '-f', '/home/worker/workspace/build/tests/bin/components/httpd.js', '-e', "const _PROFILE_PATH = '/tmp/tmpQNmhY_.mozrunner'; const _SERVER_PORT = '8888'; const _SERVER_ADDR = '127.0.0.1'; const _TEST_PREFIX = undefined; const _DISPLAY_RESULTS = false;", '-f', '/home/worker/workspace/build/tests/mochitest/server.js']
 14:21:56     INFO -  runtests.py | Server pid: 1198
 14:21:56     INFO -  runtests.py | Websocket server pid: 1201
 14:21:56     INFO -  runtests.py | websocket/process bridge pid: 1206
 14:22:06     INFO -  0 ERROR runtests.py | Timed out while waiting for websocket/process bridge startup.
 14:22:06     INFO -  Stopping web server
 14:22:06     INFO -  Stopping web socket server
 14:22:06     INFO -  Stopping websocket/process bridge
 14:22:06     INFO -  websocket/process bridge listening on port 8191
 14:22:06     INFO -  Stopping web server
 14:22:06     INFO -  Stopping web socket server
14:22:06 INFO - Stopping websocket/process bridge
Flags: needinfo?(docfaraday)
Huh. How did a rebase cause that?
Ah, someone yanked the "import socket" out of runtests.py, which we needed.
Flags: needinfo?(docfaraday)
Comment on attachment 8730340 [details]
MozReview Request: Bug 1231975 - Part 1: Basic audio mochitests for NAT scenarios. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39815/diff/17-18/
Comment on attachment 8741361 [details]
MozReview Request: Bug 1231975 - Part 2: Break a reference cycle between NrTcpSocketIpc and TCPSocketChild, in the same manner as the UDP case. r=mcmanus

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46421/diff/5-6/
Comment on attachment 8741362 [details]
MozReview Request: Bug 1231975 - Part 3: Break a reference cycle between PendingResolution and DNSRequestChild. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46423/diff/5-6/
Comment on attachment 8741363 [details]
MozReview Request: Bug 1231975 - Part 4: Add some logging and simplification in TestNrSocket. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46425/diff/5-6/
Comment on attachment 8741517 [details]
MozReview Request: Bug 1231975 - Part 5: Fix an intermittent failure caused by the NAT simulator erroneously canceling NR_ASYNC_WAIT_READ. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46547/diff/4-5/
Comment on attachment 8743872 [details]
MozReview Request: Bug 1231975 - Part 6: Disable NAT tests on android due to bug 1266217 r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48081/diff/4-5/
Comment on attachment 8741361 [details]
MozReview Request: Bug 1231975 - Part 2: Break a reference cycle between NrTcpSocketIpc and TCPSocketChild, in the same manner as the UDP case. r=mcmanus

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46421/diff/6-7/
Attachment #8741361 - Attachment description: MozReview Request: Bug 1231975 - Part 2: Break a reference cycle between NrTcpSocketIpc and TCPSocketChild, in the same manner as the UDP case. r?mcmanus → MozReview Request: Bug 1231975 - Part 2: Break a reference cycle between NrTcpSocketIpc and TCPSocketChild, in the same manner as the UDP case. r=mcmanus
Attachment #8741362 - Attachment description: MozReview Request: Bug 1231975 - Part 3: Break a reference cycle between PendingResolution and DNSRequestChild. r?drno → MozReview Request: Bug 1231975 - Part 3: Break a reference cycle between PendingResolution and DNSRequestChild. r=drno
Attachment #8741363 - Attachment description: MozReview Request: Bug 1231975 - Part 4: Add some logging and simplification in TestNrSocket. r?drno → MozReview Request: Bug 1231975 - Part 4: Add some logging and simplification in TestNrSocket. r=drno
Attachment #8741517 - Attachment description: MozReview Request: Bug 1231975 - Part 5: Fix an intermittent failure caused by the NAT simulator erroneously canceling NR_ASYNC_WAIT_READ. r?drno → MozReview Request: Bug 1231975 - Part 5: Fix an intermittent failure caused by the NAT simulator erroneously canceling NR_ASYNC_WAIT_READ. r=drno
Comment on attachment 8741362 [details]
MozReview Request: Bug 1231975 - Part 3: Break a reference cycle between PendingResolution and DNSRequestChild. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46423/diff/6-7/
Comment on attachment 8741363 [details]
MozReview Request: Bug 1231975 - Part 4: Add some logging and simplification in TestNrSocket. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46425/diff/6-7/
Comment on attachment 8741517 [details]
MozReview Request: Bug 1231975 - Part 5: Fix an intermittent failure caused by the NAT simulator erroneously canceling NR_ASYNC_WAIT_READ. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46547/diff/5-6/
Comment on attachment 8743872 [details]
MozReview Request: Bug 1231975 - Part 6: Disable NAT tests on android due to bug 1266217 r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48081/diff/5-6/
Comment on attachment 8730340 [details]
MozReview Request: Bug 1231975 - Part 1: Basic audio mochitests for NAT scenarios. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39815/diff/18-19/
Comment on attachment 8741361 [details]
MozReview Request: Bug 1231975 - Part 2: Break a reference cycle between NrTcpSocketIpc and TCPSocketChild, in the same manner as the UDP case. r=mcmanus

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46421/diff/7-8/
Comment on attachment 8741362 [details]
MozReview Request: Bug 1231975 - Part 3: Break a reference cycle between PendingResolution and DNSRequestChild. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46423/diff/7-8/
Comment on attachment 8741363 [details]
MozReview Request: Bug 1231975 - Part 4: Add some logging and simplification in TestNrSocket. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46425/diff/7-8/
Comment on attachment 8741517 [details]
MozReview Request: Bug 1231975 - Part 5: Fix an intermittent failure caused by the NAT simulator erroneously canceling NR_ASYNC_WAIT_READ. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46547/diff/6-7/
Comment on attachment 8743872 [details]
MozReview Request: Bug 1231975 - Part 6: Disable NAT tests on android due to bug 1266217 r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48081/diff/6-7/
Target Milestone: mozilla48 → mozilla49
Backed out in https://hg.mozilla.org/mozilla-central/rev/a8d0cda0ef76 for breaking local mochitests
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla49 → ---
Looking back through unfiled failures from yesterday evening, I see https://treeherder.mozilla.org/logviewer.html#?job_id=26698670&repo=mozilla-inbound intermittently. Hopefully that can be addressed before this re-lands.
Flags: needinfo?(docfaraday)
Comment on attachment 8730340 [details]
MozReview Request: Bug 1231975 - Part 1: Basic audio mochitests for NAT scenarios. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39815/diff/19-20/
Comment on attachment 8741361 [details]
MozReview Request: Bug 1231975 - Part 2: Break a reference cycle between NrTcpSocketIpc and TCPSocketChild, in the same manner as the UDP case. r=mcmanus

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46421/diff/8-9/
Comment on attachment 8741362 [details]
MozReview Request: Bug 1231975 - Part 3: Break a reference cycle between PendingResolution and DNSRequestChild. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46423/diff/8-9/
Comment on attachment 8741363 [details]
MozReview Request: Bug 1231975 - Part 4: Add some logging and simplification in TestNrSocket. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46425/diff/8-9/
Comment on attachment 8741517 [details]
MozReview Request: Bug 1231975 - Part 5: Fix an intermittent failure caused by the NAT simulator erroneously canceling NR_ASYNC_WAIT_READ. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46547/diff/7-8/
Comment on attachment 8743872 [details]
MozReview Request: Bug 1231975 - Part 6: Disable NAT tests on android due to bug 1266217 r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48081/diff/7-8/
(In reply to Wes Kocher (:KWierso) from comment #95)
> Looking back through unfiled failures from yesterday evening, I see
> https://treeherder.mozilla.org/logviewer.html#?job_id=26698670&repo=mozilla-
> inbound intermittently. Hopefully that can be addressed before this re-lands.

   I think Part 7 should fix this.
Flags: needinfo?(docfaraday)
Comment on attachment 8746594 [details]
MozReview Request: Bug 1231975 - Part 7: Disable trickle for the NAT tests, since trickle delays can cause lower priority pairs to get selected. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49481/diff/1-2/
Attachment #8746594 - Attachment description: MozReview Request: Bug 1231975 - Part 7: Disable trickle for the NAT tests, since trickle delays can cause lower priority pairs to get selected. → MozReview Request: Bug 1231975 - Part 7: Disable trickle for the NAT tests, since trickle delays can cause lower priority pairs to get selected. r?drno
Attachment #8746594 - Flags: review?(drno)
Attachment #8746594 - Flags: review?(drno) → review+
Comment on attachment 8746594 [details]
MozReview Request: Bug 1231975 - Part 7: Disable trickle for the NAT tests, since trickle delays can cause lower priority pairs to get selected. r=drno

https://reviewboard.mozilla.org/r/49481/#review46555

::: dom/media/tests/mochitest/test_peerConnection_basicAudioNATRelay.html:31
(Diff revision 2)
> +        // Make sure we don't end up choosing the wrong thing due to delays in
> +        // trickle. Once we are willing to accept trickle after ICE success, we
> +        // can maybe wait a bit to allow things to stabilize.

How about adding a reference to a follow up bug for this?

::: dom/media/tests/mochitest/test_peerConnection_basicAudioNATSrflx.html:28
(Diff revision 2)
> +        // Make sure we don't end up choosing the wrong thing due to delays in
> +        // trickle. Once we are willing to accept trickle after ICE success, we
> +        // can maybe wait a bit to allow things to stabilize.

Same bug reference here
Comment on attachment 8730340 [details]
MozReview Request: Bug 1231975 - Part 1: Basic audio mochitests for NAT scenarios. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39815/diff/20-21/
Attachment #8746594 - Attachment description: MozReview Request: Bug 1231975 - Part 7: Disable trickle for the NAT tests, since trickle delays can cause lower priority pairs to get selected. r?drno → MozReview Request: Bug 1231975 - Part 7: Disable trickle for the NAT tests, since trickle delays can cause lower priority pairs to get selected. r=drno
Comment on attachment 8741361 [details]
MozReview Request: Bug 1231975 - Part 2: Break a reference cycle between NrTcpSocketIpc and TCPSocketChild, in the same manner as the UDP case. r=mcmanus

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46421/diff/9-10/
Comment on attachment 8741362 [details]
MozReview Request: Bug 1231975 - Part 3: Break a reference cycle between PendingResolution and DNSRequestChild. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46423/diff/9-10/
Comment on attachment 8741363 [details]
MozReview Request: Bug 1231975 - Part 4: Add some logging and simplification in TestNrSocket. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46425/diff/9-10/
Comment on attachment 8741517 [details]
MozReview Request: Bug 1231975 - Part 5: Fix an intermittent failure caused by the NAT simulator erroneously canceling NR_ASYNC_WAIT_READ. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46547/diff/8-9/
Comment on attachment 8743872 [details]
MozReview Request: Bug 1231975 - Part 6: Disable NAT tests on android due to bug 1266217 r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48081/diff/8-9/
Comment on attachment 8746594 [details]
MozReview Request: Bug 1231975 - Part 7: Disable trickle for the NAT tests, since trickle delays can cause lower priority pairs to get selected. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49481/diff/2-3/
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: