1042873 - Missing byte-order conversion in ChildDNSRecord::GetNextAddr

Status: VERIFIED FIXED

(Core :: Networking, defect)

Description

[Byron Campen [:bwc]]

http://dxr.mozilla.org/mozilla-central/source/netwerk/dns/DNSRequestChild.cpp#90

|port| is host-byte-order, NetAddr.inet.port is network-byte-order.

Comment 1

[Byron Campen [:bwc]]

Attachment: Add the appropriate byte-order conversion to ChildDNSRecord::GetNextAddr.

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

[Blocking Requested - why for this release]:

Comment 3

[Patrick McManus [:mcmanus]]

Comment on attachment 8461043 [details] [diff] [review]
Add the appropriate byte-order conversion to ChildDNSRecord::GetNextAddr.

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

lgtm.. sanity checks by matching nsSocketTransport2 behavior

Comment 4

[Byron Campen [:bwc]]

Let me make sure we don't need to pick up an include for htons here. Needinfo myself to check back on try push.

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

Comment 5

[Byron Campen [:bwc]]

Looks good, requesting checkin.

Comment 6

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/03c685fb1d75

Comment 7

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8461043 [details] [diff] [review]
Add the appropriate byte-order conversion to ChildDNSRecord::GetNextAddr.

Approval Request Comment
[Feature/regressing bug #]: N/A (likely always in the E10S/DNS-STUN/TURN)

[User impact if declined]: Inability to connect to STUN/TURN servers using DNS names instead of raw IP addresses.

[Describe test coverage new/current, TBPL]: Manual testing - it's hard to test STUN/TURN in the current setup; this should be testable under Steeplechase on desktop E10S.   Very hard to test on B2G in automation.

[Risks and why]: No risk

[String/UUID change made/needed]: none

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/03c685fb1d75

Comment 9

[Jason Smith [:jsmith]]

See https://bugzilla.mozilla.org/show_bug.cgi?id=1042345#c8 for blocking rationale.

Comment 10

[u279076]

(In reply to Randell Jesup [:jesup] from comment #7)
> [Describe test coverage new/current, TBPL]: Manual testing - it's hard to
> test STUN/TURN in the current setup; this should be testable under
> Steeplechase on desktop E10S.   Very hard to test on B2G in automation.

Nils, can you please advise if this is testable by us?

Comment 11

[Nils Ohlmeier [:drno]]

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #10)
> Nils, can you please advise if this is testable by us?

Please test it manually like this:

1a) On Desktop open a e10s browser window
1b) On B2G simply open the Firefox browser

2) Open this URL: http://mozilla.github.io/webrtc-landing/stun_test_ip.html
3) After about 5s you should see output under the headline. The output should have line likes this at the end:
  a=candidate:0 1 UDP 2130379007 10.0.0.9 60122 typ host
  a=candidate:1 1 UDP 1694236671 76.103.213.96 60122 typ srflx raddr 10.0.0.9 rport 60122
Important is that you should have at least one line with "typ host" at the end, and at least one line with "typ srflx" in it.
4) Now open this URL: http://mozilla.github.io/webrtc-landing/stun_test_ip.html
5) Again after about 5s you should see the similar output as in 3). If this output is missing any line with "typ srflx" (and only has "typ host" lines) in it that is test failure. This fix works if the output again like in step 3) contains "typ host" AND "typ srflx" lines.

Comment 12

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8461043 [details] [diff] [review]
Add the appropriate byte-order conversion to ChildDNSRecord::GetNextAddr.

Aurora+ and Beta+

Note that this fix is already clear to land on b2g32 via the 2.0+ flag.

Comment 13

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8cc35ac81ce7
https://hg.mozilla.org/releases/mozilla-beta/rev/fe0621be0dc3

Still need b2g32 (I don't have a tree handy) and decisions on ESR31 and B2G 1.4

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/fe0621be0dc3

If you want a decision on esr31/b2g30, you should probably nominate the patch for approval so it shows up on RelMan's radar.

Comment 16

[u279076]

On the latest mozilla-central (2014-08-11) I get the following output:
> Created offer: v=0
> o=Mozilla-SIPUA-34.0a1 9788 0 IN IP4 0.0.0.0
> s=SIP Call
> t=0 0
> a=ice-ufrag:c1ea9b0b
> a=ice-pwd:4608618001e85642b6cb6ac3287ad532
> a=fingerprint:sha-256 BD:8D:26:EF:34:47:14:EC:3C:18:B0:31:DF:0A:32:7E:45:D1:71:37:AF:5A:2C:A2:52:AE:86:A0:A8:95:BE:F0
> m=video 52393 RTP/SAVPF 120
> c=IN IP4 64.214.126.165
> a=rtpmap:120 VP8/90000
> a=sendrecv
> a=rtcp-fb:120 nack
> a=rtcp-fb:120 nack pli
> a=rtcp-fb:120 ccm fir
> a=setup:actpass
> a=candidate:0 1 UDP 2130379007 10.244.25.141 57711 typ host
> a=candidate:1 1 UDP 1694236671 64.214.126.165 52393 typ srflx raddr 10.244.25.141 rport 57711
> a=candidate:2 1 UDP 2129723647 192.168.172.1 60129 typ host
> a=candidate:4 1 UDP 2129264895 172.16.109.1 50386 typ host
> a=candidate:0 2 UDP 2130379006 10.244.25.141 54393 typ host
> a=candidate:1 2 UDP 1694236670 64.214.126.165 43354 typ srflx raddr 10.244.25.141 rport 54393
> a=candidate:2 2 UDP 2129723646 192.168.172.1 53863 typ host
> a=candidate:4 2 UDP 2129264894 172.16.109.1 60877 typ host
> a=rtcp-mux 

Does that look right?

Comment 17

[Nils Ohlmeier [:drno]]

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #16)
> On the latest mozilla-central (2014-08-11) I get the following output:
> > a=candidate:1 1 UDP 1694236671 64.214.126.165 52393 typ srflx raddr 10.244.25.141 rport 57711
> > a=candidate:1 2 UDP 1694236670 64.214.126.165 43354 typ srflx raddr 10.244.25.141 rport 54393
>
> Does that look right?

Yes. The two lines above indicate it is working.

Comment 18

[u279076]

Thanks Nils, I get a similar result on my Flame running yesterday's 2.0-pre build.

Comment 19

[u279076]

Verified fixed in today's Aurora and Firefox 32.0b5.

Comment 20

[Lukas Blakk [:lsblakk] use ?needinfo]

This doesn't meet ESR landing criteria.