Closed
Bug 890832
Opened 11 years ago
Closed 11 years ago
Fix test_peerConnection_bug825703.html so it doesn't hit sites outside of build infra
Categories
(Core :: WebRTC: Networking, defect)
Core
WebRTC: Networking
Tracking
()
RESOLVED
FIXED
People
(Reporter: emorley, Assigned: jib)
References
Details
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
jib
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•11 years ago
|
||
Test disabled: https://hg.mozilla.org/integration/mozilla-inbound/rev/a8a3eab7f3a5
Whiteboard: [test disabled][leave open]
Comment 2•11 years ago
|
||
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•11 years ago
|
||
Note - I'm following up over email on this.
Assignee | ||
Comment 4•11 years ago
|
||
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.
Assignee | ||
Comment 5•11 years ago
|
||
This is not to say we don't have real STUN testing needs, but that's bug 848094.
Reporter | ||
Updated•11 years ago
|
status-firefox23:
--- → affected
status-firefox24:
--- → affected
Comment 6•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a8a3eab7f3a5
Assignee | ||
Comment 7•11 years ago
|
||
Attachment #772335 -
Flags: review?(jsmith)
Reporter | ||
Comment 8•11 years ago
|
||
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•11 years ago
|
||
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?
Assignee | ||
Comment 10•11 years ago
|
||
> 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•11 years ago
|
||
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.
Attachment #772335 -
Flags: review?(jsmith) → review+
Reporter | ||
Comment 12•11 years ago
|
||
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 :-)
Assignee: nobody → jib
Status: NEW → ASSIGNED
Assignee | ||
Comment 13•11 years ago
|
||
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.
Attachment #773351 -
Flags: review+
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 14•11 years ago
|
||
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).
Reporter | ||
Comment 15•11 years ago
|
||
(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.
Updated•11 years ago
|
Attachment #772335 -
Attachment is obsolete: true
Comment 16•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/28ea4eea4908
Flags: in-testsuite+
Keywords: checkin-needed
Reporter | ||
Comment 17•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/28ea4eea4908
Updated•11 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [test disabled][leave open]
Reporter | ||
Updated•11 years ago
|
status-firefox23:
affected → ---
status-firefox24:
affected → ---
You need to log in
before you can comment on or make changes to this bug.
Description
•