890832 - Fix test_peerConnection_bug825703.html so it doesn't hit sites outside of build infra

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect)

Description

[Ed Morley [:emorley]]

Broken out from bug 812342:

(In reply to Nick Thomas [:nthomas] from comment #32)
> Ah hah! It's this test then:
> http://mxr.mozilla.org/mozilla-central/source/dom/media/tests/mochitest/
> test_peerConnection_bug825703.html?force=1#57

(In reply to Michael Henry [:tinfoil] from comment #33)
> So it's a custom stun test that uses UDP 42?  Wow!  
> 
> Since it appears we're adding (limited) STUN support, should I add this too?

(In reply to Dustin J. Mitchell [:dustin] from comment #34)
> If we want more rando-oranges when x.org's stun servers go down, yes!
> 
> It seems like we should add this flow temporarily, and file a bug to
> redirect this internally when running tests on releng systems.  Nick, does
> that make sense?  Want to file such a bug and leave the bug # here for
> :tinfoil to annotate the policy?

(In reply to Ed Morley (Away until 8th July) [:edmorley UTC+1] from comment #36)
> We shouldn't allow tests that rely on third party servers, I'm going to
> disable the test for now.

Comment 1

[Ed Morley [:emorley]]

Test disabled:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8a3eab7f3a5

Comment 2

[(not currently active) Ted Mielczarek]

RelEng is in the process of actually enforcing the "no outside network connections" policy from our build and test machines via firewall rules. This test is hitting external STUN/TURN servers, which is a no-no.

Comment 3

[Jason Smith [:jsmith]]

Note - I'm following up over email on this.

Comment 4

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

The purpose of this test is solely to exercise the iceServer-url-array parsing code, which means the url's don't need to be real, only syntactically correct, for the test to succeed. - So this test doesn't actually *rely* on outside connections succeeding.

The test has been severely refactored once since its inception, but I believe this is still true, or "stun:0.0.0.0" would not succeed.

So we could change it to:

 makePC({ iceServers: [
         { url:"stun:127.0.0.1" },
         { url:"stuns:localhost", foo:"" },
         { url:"turn:[::1]:3478", username:"p", credential:"p" },
         { url:"turns:localhost:3478?transport=udp", username:"p", credential:"p" }
        ]}, true);

to prevent any outside connection attempt noise.

Comment 5

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

This is not to say we don't have real STUN testing needs, but that's bug 848094.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a8a3eab7f3a5

Comment 7

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: test_peerConnection_bug825703.html no longer hits outside sites

Comment 8

[Ed Morley [:emorley]]

Comment on attachment 772335 [details] [diff] [review]
test_peerConnection_bug825703.html no longer hits outside sites

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

::: dom/media/tests/mochitest/Makefile.in
@@ +47,5 @@
>    test_peerConnection_setLocalOfferInHaveRemoteOffer.html \
>    test_peerConnection_setRemoteAnswerInHaveRemoteOffer.html \
>    test_peerConnection_addCandidateInHaveLocalOffer.html \
>    test_peerConnection_bug822674.html \
> +  test_peerConnection_bug825703.html \

The commented out line needs to be removed as well.

Comment 9

[Jason Smith [:jsmith]]

Comment on attachment 772335 [details] [diff] [review]
test_peerConnection_bug825703.html no longer hits outside sites

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

One question inline. You also need to address what Ed has stated.

::: dom/media/tests/mochitest/test_peerConnection_bug825703.html
@@ -51,5 @@
>      makePC({ iceServers: [{ url:"http:0.0.0.0" }] }, false);
>  
>      makePC({ iceServers: [
> -                { url:"stun:0.0.0.0" },
> -                { url:"stuns:x.net", foo:"" },

Why remove these tests above? Shouldn't we have a .net & .org test case?

Comment 10

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

> Why remove these tests above? Shouldn't we have a .net & .org test case?

The parser reuses large parts of nsURI class to do the uri host/path part, so I am not concerned with coverage there. The custom code paths are for the four schemes, ip vs fqdn and port#.

Comment 11

[Jason Smith [:jsmith]]

Comment on attachment 772335 [details] [diff] [review]
test_peerConnection_bug825703.html no longer hits outside sites

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

Okay. review+ then with removing the comments Ed mentioned.

Comment 12

[Ed Morley [:emorley]]

Note this will need to land on both aurora and beta before this Saturday to avoid breakage due to bug 891375. Since this is a test only change, you can land with a=test-only :-)

Comment 13

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: test_peerConnection_bug825703.html no longer hits outside sites (2) r=jsmith

Update. Carrying forward r+ from jsmith.

Try - https://tbpl.mozilla.org/?tree=Try&rev=bce50aed90ab

> The commented out line needs to be removed as well.

Thanks for spotting that. Removed.

> Note this will need to land on both aurora and beta before this Saturday to
> avoid breakage due to bug 891375. Since this is a test only change, you can
> land with a=test-only :-)

This is good to go, but I'll need help with uplift as I only have level 1 access.

Comment 14

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Again, this is an iceServer uri parsing test only, so even though things go out on the wire as a side-effect, a switch to Default Deny wont cause the test to fail. So the reason to uplift I guess would be to get rid of the log noise (port 42 etc).

Comment 15

[Ed Morley [:emorley]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #14)
> Again, this is an iceServer uri parsing test only, so even though things go
> out on the wire as a side-effect, a switch to Default Deny wont cause the
> test to fail. So the reason to uplift I guess would be to get rid of the log
> noise (port 42 etc).

Ah of course, in which case no uplift needed.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/28ea4eea4908

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/28ea4eea4908