Mochitests for NAT scenarios

RESOLVED FIXED in Firefox 49

Status

()

P1
normal
Rank:
15
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: bwc, Assigned: bwc)

Tracking

Trunk
mozilla49
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45 affected, firefox49 fixed)

Details

Attachments

(7 attachments)

58 bytes, text/x-review-board-request
drno
: review+
Details
58 bytes, text/x-review-board-request
mcmanus
: review+
Details
58 bytes, text/x-review-board-request
drno
: review+
Details
58 bytes, text/x-review-board-request
drno
: review+
Details
58 bytes, text/x-review-board-request
drno
: review+
Details
58 bytes, text/x-review-board-request
drno
: review+
Details
58 bytes, text/x-review-board-request
drno
: review+
Details
Comment hidden (empty)
(Assignee)

Updated

3 years ago
Depends on: 1231977
(Assignee)

Updated

3 years ago
Assignee: nobody → docfaraday
(Assignee)

Comment 2

3 years ago
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/
(Assignee)

Comment 3

3 years ago
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)
(Assignee)

Comment 5

3 years ago
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.
(Assignee)

Comment 9

3 years ago
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
(Assignee)

Updated

3 years ago
Depends on: 1231981
(Assignee)

Comment 10

3 years ago
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/
(Assignee)

Updated

3 years ago
Depends on: 1258836
(Assignee)

Comment 11

3 years ago
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/
(Assignee)

Comment 12

3 years ago
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/
(Assignee)

Comment 13

3 years ago
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/
(Assignee)

Comment 14

3 years ago
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/
(Assignee)

Comment 15

3 years ago
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/
(Assignee)

Comment 16

3 years ago
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/
(Assignee)

Comment 17

3 years ago
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/
(Assignee)

Comment 34

3 years ago
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)
(Assignee)

Comment 37

3 years ago
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
(Assignee)

Comment 42

3 years ago
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/
(Assignee)

Comment 43

3 years ago
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/
(Assignee)

Comment 44

3 years ago
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/
(Assignee)

Comment 45

3 years ago
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?
(Assignee)

Comment 47

3 years ago
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.
(Assignee)

Updated

3 years ago
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+
(Assignee)

Comment 50

3 years ago
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/
(Assignee)

Comment 51

3 years ago
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/
(Assignee)

Comment 52

3 years ago
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/
(Assignee)

Comment 53

3 years ago
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/
(Assignee)

Comment 54

3 years ago
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/
(Assignee)

Comment 55

3 years ago
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/
(Assignee)

Comment 56

3 years ago
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/
(Assignee)

Comment 57

3 years ago
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/
(Assignee)

Comment 58

3 years ago
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/
(Assignee)

Comment 59

3 years ago
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/
(Assignee)

Comment 60

3 years ago
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/
(Assignee)

Comment 61

3 years ago
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/
(Assignee)

Comment 62

3 years ago
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
(Assignee)

Comment 63

3 years ago
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/
(Assignee)

Comment 64

3 years ago
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/
(Assignee)

Comment 65

3 years ago
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/
(Assignee)

Comment 66

3 years ago
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/
(Assignee)

Comment 67

3 years ago
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/
(Assignee)

Comment 68

3 years ago
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)
(Assignee)

Comment 73

3 years ago
Huh. How did a rebase cause that?
(Assignee)

Comment 74

3 years ago
Ah, someone yanked the "import socket" out of runtests.py, which we needed.
Flags: needinfo?(docfaraday)
(Assignee)

Comment 75

3 years ago
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/
(Assignee)

Comment 76

3 years ago
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/
(Assignee)

Comment 77

3 years ago
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/
(Assignee)

Comment 78

3 years ago
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/
(Assignee)

Comment 79

3 years ago
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/
(Assignee)

Comment 80

3 years ago
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/
(Assignee)

Comment 81

3 years ago
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
(Assignee)

Comment 82

3 years ago
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/
(Assignee)

Comment 83

3 years ago
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/
(Assignee)

Comment 84

3 years ago
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/
(Assignee)

Comment 85

3 years ago
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/
(Assignee)

Comment 86

3 years ago
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/
(Assignee)

Comment 87

3 years ago
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/
(Assignee)

Comment 88

3 years ago
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/
(Assignee)

Comment 89

3 years ago
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/
(Assignee)

Comment 90

3 years ago
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/
(Assignee)

Comment 91

3 years ago
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 → ---
status-firefox49: fixed → affected
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)
(Assignee)

Comment 96

3 years ago
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/
(Assignee)

Comment 97

3 years ago
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/
(Assignee)

Comment 98

3 years ago
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/
(Assignee)

Comment 99

3 years ago
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/
(Assignee)

Comment 100

3 years ago
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/
(Assignee)

Comment 101

3 years ago
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/
(Assignee)

Comment 103

3 years ago
(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.
(Assignee)

Updated

3 years ago
Flags: needinfo?(docfaraday)
(Assignee)

Comment 104

3 years ago
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
(Assignee)

Comment 106

3 years ago
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
(Assignee)

Comment 107

3 years ago
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/
(Assignee)

Comment 108

3 years ago
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/
(Assignee)

Comment 109

3 years ago
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/
(Assignee)

Comment 110

3 years ago
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/
(Assignee)

Comment 111

3 years ago
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/
(Assignee)

Comment 112

3 years ago
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.