720846 - scriptable PRNetAddr / script access to nsSocketTransport self address.

Status: RESOLVED FIXED

(Core :: Networking, enhancement)

Description

[Derrick Rice (:drice)]

nsISocketTransport does not provide a means of non-native extensions of discovering the connection details of the socket connection.  Specifically the local port number (for my use case), though it would be nice if all connection information could be gleamed from a scripted interface.

This request is to expand the nsISocketTransport interface to include getters (either functions or read-only attributes) for the connection information which can otherwise only be seen in getPeerAddr and getSelfAddr.

I'll have more details of the proposed change after I get more familiar with the implementation.

Comment 1

[Derrick Rice (:drice)]

I'm considering creating a new interface to represent information within PRNetAddr.  An instance of this interface could be created by passing a PRNetAddr, and the full information would then be accessible via a scriptable interface with the following (tentative/proposed) fields:

- int family (corresponding to a constant below)
- int port (for INET and INET6)
- string path (for LOCAL/UNIX)
- int ipv4 (for INET)
- int[4] ipv6 (for INET6*)
- string ipString (stringified version of ipv4 / ipv6)
- int flowinfo (for INET6)
- int scope_id (for INET6)

And these constants (again, open to suggestion on names)

- AF_LOCAL / AF_UNIX
- AF_INET
- AF_INET6

* I'm not very familiar with the various types available in XPCOM idl, so there may be a more appropriate container for ipv6 addresses. long[2]?  long long?

Of course this change comes with the necessary overhead of having to update the IDL and implement conversion logic whenever new address families are added, though I take it that is rare :)

[--
As a note mostly to myself, Josh (:jdm) suggested pinging biesi (netwerk module owner) and/or jduell (module peer and original contributor of get*Addr)
--]

Comment 2

[Christian :Biesinger (don't email me, ping me on IRC)]

Sounds like a good idea to me. For the IP addresses, I'd just make it a string, that's what we do elsewhere, e.g. http://mxr.mozilla.org/mozilla-central/source/netwerk/dns/nsIDNSRecord.idl#72 and http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsIHttpChannelInternal.idl#119

Comment 3

[Derrick Rice (:drice)]

So I've made progress on this, but I've got the following blocking issues:

* Is there an existing implementation of INET or INET6 address to nsACString?  If so, then life is good.

* If not, then is there at least a decision on ipv6 textual representation, yet?  It's not as simple a question as it ought to be: http://tools.ietf.org/html/rfc5952

* And if not, then that's probably a bigger issue that netwerk maintainers should be deciding - not me.  I've seen the difficulty that multiple ipv6 text representations can cause.

Via IRC, Dagger says:

" yeah, [http://tools.ietf.org/html/rfc5952 is] the one. getnameinfo() produces addresses in that format, and is what you're supposed to use to get them (but it's a POSIX function and takes "struct sockaddr"s rather than whatever Mozilla uses) "

Comment 4

[Honza Bambas (:mayhemer)]

(In reply to Derrick Rice (:drice) from comment #3)
> So I've made progress on this, but I've got the following blocking issues:

Check http://www.mozilla.org/projects/nspr/reference/html/prntdb.html#20510

Comment 5

[Derrick Rice (:drice)]

Attachment: preliminary patch 01

Here's a preliminary set of changes which creates:

* nsINetAddr, a new scriptable interface
* nsNetAddr, the default implementation, which is constructed from a PRNetAddr and makes a private copy.

There's one 'XXX' in there regarding the use of nsACString.Assign. I'm not sure if I have to do SetCapacity first.

I think the next thing I need to do is create some automated tests, which I'm reading docs on now:
https://developer.mozilla.org/en/Mozilla_automated_testing

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 598248 [details] [diff] [review]
preliminary patch 01

+    aAddress.SetCapacity(16);
+    aAddress.SetCapacity(46);

You need to check that for success:

  if (!aAddress.SetCapacity(16))
    return NS_ERROR_OUT_OF_MEMORY;

+  case PR_AF_LOCAL:
+    // XXX: I'm not super familiar with the nsString stuff.  Is the below safe?
+    // Do I need to SetCapacity first? path can be 104 or 108 long, depending on
+    // OS.
+    aAddress.Assign(mAddr.local.path);

nope, no need for SetCapacity

Comment 7

[Derrick Rice (:drice)]

Remark from https://developer.mozilla.org/en/NsACString/Assign:

"If insufficient memory is available to perform the assignment, then the string's internal buffer will point to a static empty (zero-length) buffer."

Which, when I look at http://mxr.mozilla.org/mozilla-central/source/xpcom/string/src/nsTSubstring.cpp#316, I don't actually see happening.  ReplacePrep does not truncating, and there's no false branch handling it there.

I pinged biesi on IRC and he suggested leaving as-is, since the current pattern of use with Assign is to ignore the possibility of failure.  FWIW, detecting failure would be contrived since I would need to fire Truncate the string, then Assign and verify that the string is not still truncated.  This is undesirable because it introduces unnecessary modifications to the string.



I'm working on automated tests now.  I'm constructing tests that...

* For each family, create an nsNetAddr from a PRNetAddr and ... 
  - QI it to nsINetAddr
  - ensure the output of the getters is expected
  - ensure that inappropriate getters return NS_ERROR_UNAVAILABLE
* Ensure that nsNetAddr creates a copy, not a reference to a given PRNetAddr
* Ensure that nsNetAddr with an invalid family creates NS_ERROR_UNEXPECTED for getters.

They may be rather trivial, but if only to give myself experience creating automated tests, I'd like to complete them.  I'm rusty in C++ and have no experience QI'ing and invoking XPCOM interface methods from C++.

Comment 8

[Derrick Rice (:drice)]

Unfortunately I cannot write cpp unit tests because netwerk doesn't export symbols.  According to khuey, this has to be tested via an XPCOM service or not at all.  I'll get to work on nsISocketTransport, then.

Comment 9

[Derrick Rice (:drice)]

Attachment: untested patch 01

Full implementation, including getters for nsISocketTransport.  I haven't been able to test this at all, because the SocketTransport test doesn't build out of the box.  Not sure what I need to do in order to use it (hoping biesi can help here)

Comment 10

[Derrick Rice (:drice)]

Attachment: patch and tests 01

So I realized, as I was writing tests (go figure!) that I had neglected to consider byte ordering.  Included is this attachment is an up-to-date patch which also includes xpcshell tests of nsNetAddr via nsISocketTransport.

Unfortunately, there are quirks with ipv6 addresses that can be represented also with ipv4 addressed.  My machine (I assume it's OS dependent) likes to make loopback connections from ::ffff:127.0.0.1 rather than ::1.  In addition, my machine likes to provide ipv4 connections to socket listeners when the peer is using an ipv4 embedded address.  So the server sees the client as 127.0.0.1/FAMILY_INET rather than ::ffff:127.0.0.1/FAMILY_INET6

I've left out the checks in that portion of the xpcshell test.

Comment 11

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 600221 [details] [diff] [review]
patch and tests 01

+++ b/netwerk/base/src/nsNetAddr.cpp
+/*
+ * Original implementation via Bug 720846.
+ */

That's not a useful comment - people can always find it via the mercurial history. Just delete it here.

Comment 12

[Derrick Rice (:drice)]

Attachment: patch and tests 02

Changed as requested.  I'm not sure if I should be requesting that this new patch be reviewed, considering that its exactly the changes requested on top of a patch which was review+.

What else do I need to do in order to see this through to inclusion in Nightly?

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 601087 [details] [diff] [review]
patch and tests 02

s/an FAMILY/a FAMILY/g, and sr=me

Comment 14

[Derrick Rice (:drice)]

Attachment: patch and tests 03

Corrected comments, as requested by bz.

$ diff attachment.cgi\?id\=601087 .hg/patches/scriptnetaddr 
98c98
< +     * @return the port number for an FAMILY_INET or FAMILY_INET6 address.
---
> +     * @return the port number for a FAMILY_INET or FAMILY_INET6 address.
106c106
< +     * @return the flow label for an FAMILY_INET6 address. 
---
> +     * @return the flow label for a FAMILY_INET6 address. 
115c115
< +     * @return the address scope of an FAMILY_INET6 address.  
---
> +     * @return the address scope of a FAMILY_INET6 address.

Comment 15

[Josh Matthews [:jdm]]

Congratulations, and thanks Derrick!

http://hg.mozilla.org/integration/mozilla-inbound/rev/6d239fd74f71

Comment 16

[Ed Morley [:emorley]]

Unfortunately backed out for Windows builds failures:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=6d239fd74f71
https://tbpl.mozilla.org/php/getParsedLog.php?id=9690404&tree=Mozilla-Inbound
{
e:/builds/moz2_slave/m-in-w32-dbg/build/netwerk/base/src/nsNetAddr.cpp(90) : error C2039: 'local' : is not a member of 'PRNetAddr'

        e:\builds\moz2_slave\m-in-w32-dbg\build\obj-firefox\dist\include\prio.h(177) : see declaration of 'PRNetAddr'

e:/builds/moz2_slave/m-in-w32-dbg/build/netwerk/base/src/nsNetAddr.cpp(90) : error C2228: left of '.path' must have class/struct/union
}

https://hg.mozilla.org/integration/mozilla-inbound/rev/6f522f01319c

Comment 17

[Derrick Rice (:drice)]

Attachment: patch and tests 04

Needed to guard the use of |local| in the same way that prio.h does.  That's included in this patch.  Unfortunately, I don't have a windows build environment set up.  I can do so if strictly necessary, but I think the fix is obvious (I'll let a reviewer decide).

Also, my test hung:

https://tbpl.mozilla.org/php/getParsedLog.php?id=9691615&tree=Mozilla-Inbound

It runs nicely on my own Ubuntu machine.  I added a try/catch handler in a portion of JS code which is under C/C++ callbacks, since its apparently possible for errors to occur there and not show up in the test log.  I also added a cleanup routine to shut down the TestServer.  We'll see this generates more information if/when it hangs again.

review and checkin requested. (Am I breaking a rule by requesting them at the same time?)

Comment 18

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 601328 [details] [diff] [review]
patch and tests 04

Seems jdm knows our test harness much better than me, so passing it to him.

Just one comment from me: Generally style is to put a space between "try" and the bracket

Comment 19

[Josh Matthews [:jdm]]

Comment on attachment 601328 [details] [diff] [review]
patch and tests 04

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

Let's fix these up and push to try.

::: netwerk/test/unit/test_net_addr.js
@@ +41,5 @@
> +    } catch(e) {
> +      /* In a native callback such as onSocketAccepted, exceptions might not
> +       * get output correctly or logged to test output. Send them through
> +       * do_throw, which fails the test immediately. */
> +      do_throw("Unexpected exception: " + e);

do_report_unexpected_exception(e, "descriptive text")

@@ +62,5 @@
> +    this.peerAddr = null;
> +  } ,
> +
> +  stop: function() {
> +    try { this.reset(); } catch(ignore) {}

Remove the unnecessary try/catch.

@@ +159,5 @@
> +  transport = sts.createTransport(null, 0, '::1', serv.port, null);
> +  transport.openOutputStream(Ci.nsITransport.OPEN_BLOCKING,0,0);
> +}
> +
> +function run_test() {

This function needs a do_test_pending() or we might go crazy. I'm not entirely sure how this has been working.

Comment 20

[Derrick Rice (:drice)]

Attachment: patch and tests 04

I'm not using do_test_pending/do_test_finished - I'm using add_test/run_next_test.

openOutputStream should trigger onSocketAccept, then acceptCallback, which will get around to run_next_test (or fail).

I've made the other requested changes.

Comment 21

[Josh Matthews [:jdm]]

Comment on attachment 601348 [details] [diff] [review]
patch and tests 04

>+  transport.openOutputStream(Ci.nsITransport.OPEN_BLOCKING,0,0);
>+  // openOutputStream -> onSocketAccepted -> acceptedCallback -> run_next_test

I'm not convinced that openOutputStream will end up synchronously calling onSocketAccepted before returning to the event loop. I would like to add a do_test_pending in run_test, as well as add_test(do_test_finished) to make sure we don't run into any problems. Run the test locally to make sure I'm not crazy as well :)

Comment 22

[Josh Matthews [:jdm]]

Comment on attachment 601348 [details] [diff] [review]
patch and tests 04

Psyke, Derrick was kind enough to explain how I was wrong. Pushing to try!

Comment 23

[Mozilla RelEng Bot]

Autoland Failure
 There are no patches to run.

Comment 24

[Mozilla RelEng Bot]

Autoland Patchset:
	Patches: 601348
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=e04add934c41
Try run started, revision e04add934c41. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=e04add934c41

Comment 25

[Derrick Rice (:drice)]

Attachment: xpcshell-test debugging patch 01

So the test is still hanging.

https://tbpl.mozilla.org/php/getParsedLog.php?id=9700096&tree=Try

I asked khuey if there was some way to get the verbose output out of the test, and his suggestion was to use EXTRA_TEST_ARGS=--verbose , but I'm worried that it will punish the build system, since every test would then be spewing out log lines.

An alternative from khuey was to add a test to the end which fails, triggering the xpcshell-test framework to print the verbose information.  I took this suggestion, plus some hackery into _tests_pending to print out how many tests were pending, to see if it was a mismatch issue with pending/finished.

This patch follows the existing patch and adds do_print statements and the above mentioned intentional failure after a 10 second delay.

jdm, can we put this back into Try just for Linux dbg? (Linux dbg was the fastest build)

Comment 26

[Mozilla RelEng Bot]

Try run for e04add934c41 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=e04add934c41
Results (out of 25 total builds):
    exception: 2
    success: 13
    warnings: 10
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-e04add934c41

Comment 27

[Mozilla RelEng Bot]

Autoland Patchset:
	Patches: 601426
	Branch: mozilla-central => try
Error applying patch 601426 to mozilla-central.
unable to find 'netwerk/test/unit/test_net_addr.js' for patching
3 out of 3 hunks FAILED -- saving rejects to file netwerk/test/unit/test_net_addr.js.rej
abort: patch failed to apply

Could not apply and push patchset:

Comment 28

[Mozilla RelEng Bot]

Autoland Patchset:
	Patches: 601348, 601426
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=c9ac603da23b
Try run started, revision c9ac603da23b. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=c9ac603da23b

Comment 29

[Mozilla RelEng Bot]

Try run for c9ac603da23b is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=c9ac603da23b
Results (out of 3 total builds):
    success: 2
    warnings: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-c9ac603da23b

Comment 30

[Josh Matthews [:jdm]]

Looks like we never get a client connection for the ipv6 test for some reason.

Comment 31

[Derrick Rice (:drice)]

The ipv6 connection is not being established.  I looked a little further into this, and got some help from bhearsum in IRC.  He was able to determine that the Try servers do have ipv6 looback addresses:

lo        Link encap:Local Loopback  
          inet addr:127.0.0.1  Mask:255.0.0.0
          inet6 addr: ::1/128 Scope:Host
          UP LOOPBACK RUNNING  MTU:16436  Metric:1
          RX packets:39 errors:0 dropped:0 overruns:0 frame:0
          TX packets:39 errors:0 dropped:0 overruns:0 carrier:0
          collisions:0 txqueuelen:0 
          RX bytes:3993 (3.8 KiB)  TX bytes:3993 (3.8 KiB)

However, it turns out that my machines doesn't

lo        Link encap:Local Loopback  
          inet addr:127.0.0.1  Mask:255.0.0.0
          UP LOOPBACK RUNNING  MTU:16436  Metric:1
          RX packets:2585029 errors:0 dropped:0 overruns:0 frame:0
          TX packets:2585029 errors:0 dropped:0 overruns:0 carrier:0
          collisions:0 txqueuelen:0 
          RX bytes:174997179 (174.9 MB)  TX bytes:174997179 (174.9 MB)

Something in the netwerk/network stack is allowing me to make connections to ::1 on my local machine, though.  

In any case, the failure is related to ipv6 issues.  None of the other netwerk tests are using ipv6 yet, and I am not the right person to be trailblazing on that.  I'll be submitting a patch which drops the ipv6 test and also puts better protection in place to avoid the test hanging (as well as some commenting).

Sorry to let anyone down who was counting on getting ipv6 testing out of this :)

Comment 32

[Derrick Rice (:drice)]

Attachment: patch and tests 05

Adjusted the test case and hg pulled/updated.  Here's the latest patch including all changes.  Only the test should have been modified since the previous patch (04).

Hoping this is the last touch here; the testing has been more difficult than it has felt like it was worth.

Comment 33

[Josh Matthews [:jdm]]

Comment on attachment 601586 [details] [diff] [review]
patch and tests 05

I appreciate your perseverance here! However, I don't think we want the timeout in this test on a permanent basis - it's useful for debugging, but if the machine hiccoughs and doesn't connect for 6 seconds then we'll have spurious failures. I'll take it out when I check it in.

Comment 34

[Josh Matthews [:jdm]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/d0477ba39a89

Comment 35

[Marco Bonardo [:mak]]

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