Closed
Bug 1231975
Opened 9 years ago
Closed 8 years ago
Mochitests for NAT scenarios
Categories
(Core :: WebRTC: Networking, defect, P1)
Core
WebRTC: Networking
Tracking
()
RESOLVED
FIXED
mozilla49
People
(Reporter: bwc, Assigned: bwc)
References
Details
Attachments
(7 files)
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 |
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 |
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 |
No description provided.
Updated•9 years ago
|
Rank: 15
Assignee | ||
Comment 1•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/39815/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39815/
Attachment #8730340 -
Flags: review?(drno)
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → docfaraday
Assignee | ||
Comment 2•9 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•9 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 4•9 years ago
|
||
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•9 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 6•9 years ago
|
||
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+
Comment 7•9 years ago
|
||
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.
Comment 8•9 years ago
|
||
(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•9 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 | ||
Comment 10•9 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 | ||
Comment 11•9 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•9 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•9 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•9 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•9 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•9 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•9 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 18•9 years ago
|
||
Assignee | ||
Comment 19•9 years ago
|
||
Assignee | ||
Comment 20•9 years ago
|
||
Assignee | ||
Comment 21•9 years ago
|
||
Assignee | ||
Comment 22•9 years ago
|
||
Assignee | ||
Comment 23•9 years ago
|
||
Assignee | ||
Comment 24•9 years ago
|
||
Assignee | ||
Comment 25•9 years ago
|
||
Assignee | ||
Comment 26•9 years ago
|
||
Assignee | ||
Comment 27•9 years ago
|
||
Assignee | ||
Comment 28•9 years ago
|
||
Assignee | ||
Comment 29•9 years ago
|
||
Assignee | ||
Comment 30•9 years ago
|
||
Assignee | ||
Comment 31•9 years ago
|
||
Assignee | ||
Comment 32•9 years ago
|
||
Assignee | ||
Comment 33•9 years ago
|
||
Assignee | ||
Comment 34•9 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 35•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/46423/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46423/
Assignee | ||
Comment 36•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/46425/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46425/
Assignee | ||
Comment 37•9 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 38•9 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
https://reviewboard.mozilla.org/r/46421/#review43035
Attachment #8741361 -
Flags: review?(mcmanus) → review+
Comment 39•9 years ago
|
||
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+
Updated•9 years ago
|
Attachment #8741362 -
Flags: review?(drno) → review+
Comment 40•9 years ago
|
||
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 41•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/46547/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46547/
Attachment #8741517 -
Flags: review?(drno)
Assignee | ||
Comment 42•9 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•9 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•9 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•9 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/
Updated•9 years ago
|
Attachment #8741517 -
Flags: review?(drno) → review+
Comment 46•9 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
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•9 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 | ||
Comment 48•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/48081/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/48081/
Assignee | ||
Updated•8 years ago
|
Attachment #8743872 -
Flags: review?(drno)
Comment 49•8 years ago
|
||
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•8 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•8 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•8 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•8 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•8 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•8 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•8 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•8 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•8 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•8 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•8 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•8 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•8 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•8 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•8 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•8 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•8 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•8 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•8 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
Comment 69•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/4cff2bed6747
https://hg.mozilla.org/integration/mozilla-inbound/rev/8aa793156ad0
https://hg.mozilla.org/integration/mozilla-inbound/rev/1913c67263eb
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d0c273048d6
https://hg.mozilla.org/integration/mozilla-inbound/rev/b21f8efbd341
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa8850420313
Assignee | ||
Comment 70•8 years ago
|
||
Comment 71•8 years ago
|
||
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)
Comment 72•8 years ago
|
||
Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/90ebcb995507
https://hg.mozilla.org/integration/mozilla-inbound/rev/97812e66ab11
https://hg.mozilla.org/integration/mozilla-inbound/rev/64535324b3af
https://hg.mozilla.org/integration/mozilla-inbound/rev/96fecd75d2da
https://hg.mozilla.org/integration/mozilla-inbound/rev/91ae125d6928
https://hg.mozilla.org/integration/mozilla-inbound/rev/15a7a3728b50
Assignee | ||
Comment 73•8 years ago
|
||
Huh. How did a rebase cause that?
Assignee | ||
Comment 74•8 years ago
|
||
Ah, someone yanked the "import socket" out of runtests.py, which we needed.
Flags: needinfo?(docfaraday)
Assignee | ||
Comment 75•8 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•8 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•8 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•8 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•8 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•8 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•8 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•8 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•8 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•8 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•8 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•8 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•8 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•8 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•8 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•8 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•8 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/
Comment 92•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae1dc75009e7
https://hg.mozilla.org/integration/mozilla-inbound/rev/29dce05596c6
https://hg.mozilla.org/integration/mozilla-inbound/rev/7092e498ac3a
https://hg.mozilla.org/integration/mozilla-inbound/rev/408df9f84697
https://hg.mozilla.org/integration/mozilla-inbound/rev/d56adef9c8e0
https://hg.mozilla.org/integration/mozilla-inbound/rev/8ed054e5853e
Comment 93•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ae1dc75009e7
https://hg.mozilla.org/mozilla-central/rev/29dce05596c6
https://hg.mozilla.org/mozilla-central/rev/7092e498ac3a
https://hg.mozilla.org/mozilla-central/rev/408df9f84697
https://hg.mozilla.org/mozilla-central/rev/d56adef9c8e0
https://hg.mozilla.org/mozilla-central/rev/8ed054e5853e
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Updated•8 years ago
|
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)
Assignee | ||
Comment 96•8 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•8 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•8 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•8 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•8 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•8 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 102•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/49481/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/49481/
Assignee | ||
Comment 103•8 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•8 years ago
|
Flags: needinfo?(docfaraday)
Assignee | ||
Comment 104•8 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)
Updated•8 years ago
|
Attachment #8746594 -
Flags: review?(drno) → review+
Comment 105•8 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
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•8 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•8 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•8 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•8 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•8 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•8 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•8 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/
Comment 113•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae8fa686d02b
https://hg.mozilla.org/integration/mozilla-inbound/rev/6202e1085f04
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc299d343111
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e24ee7fdae8
https://hg.mozilla.org/integration/mozilla-inbound/rev/dfa893ec9ad5
https://hg.mozilla.org/integration/mozilla-inbound/rev/9384b3767131
https://hg.mozilla.org/integration/mozilla-inbound/rev/8dad9ad634a3
Comment 114•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ae8fa686d02b
https://hg.mozilla.org/mozilla-central/rev/6202e1085f04
https://hg.mozilla.org/mozilla-central/rev/bc299d343111
https://hg.mozilla.org/mozilla-central/rev/8e24ee7fdae8
https://hg.mozilla.org/mozilla-central/rev/dfa893ec9ad5
https://hg.mozilla.org/mozilla-central/rev/9384b3767131
https://hg.mozilla.org/mozilla-central/rev/8dad9ad634a3
Status: REOPENED → RESOLVED
Closed: 8 years ago → 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in
before you can comment on or make changes to this bug.
Description
•