526207 - Add IP address and port info to nsIHttpActivityObserver

Status: RESOLVED FIXED

(Core :: Networking: HTTP, enhancement)

Description

[Matthew Gertner]

Currently there is no way to determine the client IP/port and server IP/port (well, maybe the server port) for an HTTP channel. I need this for a project, and apparently the Firebug folks would like it as well.

My proposal is to add this information to the extra parameters of nsIHttpActivityObserver::observeActivity:

subtype = STATUS_CONNECTING_TO, aExtraSizeData = server port, aExtraStringData = server IP
subtype = STATUS_CONNECTED_TO, aExtraSizeData = client port, aExtraStringData = client IP

Comments welcome.

Comment 1

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Yes, this would be great to have for Firebug.
Honza

Comment 2

[carglue a t ya hoo do tcom]

Attachment: nsSocketTransport2.cpp patch for passing IP and port via notification

Comment 3

[carglue a t ya hoo do tcom]

DejaClick has been using the http-activity-distributor interface on our backend for several years (see bug 488270) and we were involved in the effort (along with a guy from IBM) to get the original interface created.  We also had to address the missing IP and port issue, so we ended up patching Firefox source (see attached patch) and extracting the extra payload from the aExtraSizeData param via our JS code as follows:

   // convert decimal IP address & port into individual octets (in reverse order) and port
   extractNetInfo : function( aDecimalAddrPort )
   {
      var s = aDecimalAddrPort.toString();
      var addr = Number(s.slice(0,-5));
      var port = Number(s.slice(-5));
      return {ipaddr:((addr << 24) >>> 24) + "." + ((addr << 16) >>> 24) + "." + ((addr << 8) >>> 24) + "." + (addr >>> 24), ipport:port};
   },

...

   case STATUS_CONNECTING_TO:

      var netInfo = gDCExtras.extractNetInfo( aExtraSizeData );
      stepTiming.ipaddr = netInfo.ipaddr;
      stepTiming.ipport = netInfo.ipport;

...

   case STATUS_CONNECTED_TO:
      if (!stepTiming.connectStart || !stepTiming.ipaddr) {
         // When keep-alive is enabled, we don't get a STATUS_CONNECTING_TO
         // event that we can receive the connection's IP and port from. Also,
         // it appears that it is not possible to use the property bag to store
         // and retrieve the IP and port because a new channel will usually be
         // created (even though the underlying socket connetion may be reused)
         // so the information gets lost.  Thus, hacking Firefox itself to tell
         // us what IP and port that a given connection is using was the only
         // quick alternative.
         var netInfo = gDCExtras.extractNetInfo( aExtraSizeData );
         stepTiming.ipaddr = netInfo.ipaddr;
         stepTiming.ipport = netInfo.ipport;
      }

...

Our FF patch only works for IPV4-style IPs (due to size restrictions) and was really just a quick and dirty hack, but there were very few options available to us at the time (without a lot of Firefox code restructuring) to pass the IP and port all the way up from deep in the netwerk socket layer as an observer notification.

It would sure be nice to finally have this patch (or something like it) become part of the Firefox source so we don't have to keep patching.  =)

Comment 4

[carglue a t ya hoo do tcom]

For posterity, please see the following related bugs:

https://bugzilla.mozilla.org/show_bug.cgi?id=488270
https://bugzilla.mozilla.org/show_bug.cgi?id=308371
https://bugzilla.mozilla.org/show_bug.cgi?id=341702

Comment 5

[Honza Bambas (:mayhemer)]

Matt, Honza:
I am not sure what you mean by the "client" IP and port. Is the address of the interface and port that has been bound on the client machine for the connection? Or has it to be an external IP that the server sees? The former is possible but needs a C++ change to nsHttpConnection class. The letter is impossible.

carglue a t ya hoo do tcom:
I want to keep the proposed approach to pass this information through the activity observer, it is much more suitable for this, with no performance and compatibility effects. Do you see any reason why not to?

Comment 6

[Jan Honza Odvarko [:Honza] (always need-info? me)]

In Firebug, I have a request for displaying IP address of the server that was connected.

See also:
http://code.google.com/p/fbug/issues/detail?id=654

Honza

Comment 7

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

Comment on attachment 410886 [details] [diff] [review]
nsSocketTransport2.cpp patch for passing IP and port via notification

this patch is a horrible hack and not necessary. the consumer of the socket transport can just query the socket transport for the data.

Wouldn't it be much simpler and cleaner to expose a getter for this data on nsIHttpChannelInternal or somesuch?

Comment 8

[carglue a t ya hoo do tcom]

Thanks for the kind words.  However, this 'horrible hack' was necessary for our own needs to provide a means to shuttle the IP & port info up from deep within the bowels of the socket layer without rewriting code in several layers of core HTTP interfaces to pass some new param.

> the consumer of the socket transport can just query the socket transport for the data
Well, unless something has changed, there were (and are) no scriptable interfaces for a passive observer to retrieve the actual IP and port used in creating the socket connection.  The other issue we had (specifically) is that we needed to know the IP and port of the socket connection even if the connection failed, and we needed to know it at the moment the socket connection was made, rather than making some async call after the fact when the connection was closed and gone.

Re: Comment #5, this patch actually does pass its information through the http activity observer interface.  We just chose to use the aExtraSizeData param only, instead of using both aExtraSizeData and aExtraStringData.  I vaugely remember there being some kind of issue with trying to use aExtraStringData for the payload transport for either the IP or port info (which would have been preferred).  But its been so long now I don't quite remember the reason.

Keep in mind that our original goal was to provide the most minimally invasive patch that would allow us to push this unavailable data up through all the networking layers without reworking a bunch of code which was likely to change.  If you folks are willing to improve the implementation by making all the necessary changes to a number of additional files, I surely would welcome it.

Comment 9

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

Sorry, my words were perhaps too harsh. Really, my point was mainly that for checking this patch into mozilla.org code, this is not the ideal approach.

That said, wouldn't my suggestion work? The HttpActivityObserver gets the channel as a parameter, and if the channel provided a getter function for the IP addresses then you should be able to easily query them. Is the problem that the notifications you get come at the wrong time? (obviously right now they also don't have the right data)

Comment 10

[carglue a t ya hoo do tcom]

Re: Comment #9, yes I believe your approach might be a better alternative overall, as long the getters are accessible from a scriptable iface and the IP and port remain the same and available from both STATUS_CONNECTING_TO and STATUS_SENDING_TO notifications, which I think they should.  This is needed since keep-alives will not send the STATUS_CONNECTING_TO or STATUS_CONNECTED_TO notifications on a reused channel, thus our first observable notification might be STATUS_SENDING_TO, at which point we need to grab the IP and port.

Comment 11

[Matthew Gertner]

(In reply to comment #5)
> Matt, Honza:
> I am not sure what you mean by the "client" IP and port. Is the address of the
> interface and port that has been bound on the client machine for the
> connection? Or has it to be an external IP that the server sees? The former is
> possible but needs a C++ change to nsHttpConnection class. The letter is
> impossible.

Let's go with the possible one then. :-)

Comment 12

[Matthew Gertner]

(In reply to comment #9)
> Sorry, my words were perhaps too harsh. Really, my point was mainly that for
> checking this patch into mozilla.org code, this is not the ideal approach.
> 
> That said, wouldn't my suggestion work? The HttpActivityObserver gets the
> channel as a parameter, and if the channel provided a getter function for the
> IP addresses then you should be able to easily query them. Is the problem that
> the notifications you get come at the wrong time? (obviously right now they
> also don't have the right data)

Having the data passed in the "extra" parameters of the nsIHttpActivityObserver is sufficient for our purposes, but I agree that adding getters to the channel somehow would be a cleaner solution. Are you suggesting a change to the nsIHttpObserver interface so that the channel is passed as a parameter to the callback?

Comment 13

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

http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/public/nsIHttpActivityObserver.idl#79
79     void observeActivity(in nsISupports  aHttpChannel,

Does that suffice? ;)

The patch here passes the data as the integer, which works but is suboptimal because a) it can't work for IPv6 and b) even when limiting yourself to IPv4 you can't include the client IP (only the server one). The string would work better. But, I still think a property of the channel would be better.

Comment 14

[Matthew Gertner]

(In reply to comment #13)
> http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/public/nsIHttpActivityObserver.idl#79
> 79     void observeActivity(in nsISupports  aHttpChannel,
> 
> Does that suffice? ;)

D'oh! Should have checked the interface before I commented. :-)

Honza, what do you think?

Comment 15

[Honza Bambas (:mayhemer)]

I have already done part of the work biesi suggests, but now I have got few more urgent blocking things. When I'm done with them I'll return back to this bug.

Comment 16

[Srirang G Doddihal (Brahmana)]

Hi all,

Wanted to let you know that I just started working on this. Had a discussion with biesi on IRC and here is what I am doing:

1) Putting 4 readonly attributes on nsIHttpChannelInternal - 2 strings for IP-Addrs and 2 longs for port numbers. These are available only after the name resolution has happened. Querying these before that will result in NS_ERROR_NOT_AVAILABLE. These attributes are reflected in the implementation as two nsCString and two PRUint32 member variables on nsHttpChannel. Also a boolean flag mHasConnectionDetails is used to decide the availability of these attributes.

2) nsHttpChannel gets transport progress events via nsITransportEventSink::OnTransportStatus(), where the nsITransport (the socketTransport in this case) is passed when called from nsHttpTransaction. This is used to get the socket transport and populate the corresponding attributes. 

3) The socket details are available for any state of nsISocketTransport except for STATE_RESOLVING and the STATE_READING & STATE_WRITING of nsITransport. I can probably just check for CONNECTING_TO or CONNECTED_TO, but AFAIK, those are not fired for keep-alive connections being reused. So I just look for any state after RESOLVING and mHasConnectionDetails == false, so that I wont try and fetch these details multiple times unnecessarily.

Comments requested.

Question:

All this is happening on the main thread. Is it OK to call functions on the socketTransport to get the IPs and port nos?

Will put up a patch soon.

Honza,
I am assigning this bug to myself, assuming you are already occupied.

Comment 17

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

Please don't add even more member variables to nsHttpChannel. Just query the socket transport in the getters.

Accessing the socket transport is OK from any thread.

Comment 18

[Srirang G Doddihal (Brahmana)]

To avoid having more members in nsHttpChannel (as it already has too many and it becomes hard to understand their interactions) three suggestions came up:

1) Put a getter on nsHttpConnection for the socketTransport -- Not possible as the nsHttpConnection object should be accessed only on socket thread.

2) Hold a reference to the nsISocketTransport in the channel. We get it in OnTransportStatus.

3) Combine the four attributes into a structure called nsHttpConnectionProperties and have an instance of that as a member variable on nsHttpChannel. This way we will not hold a reference to the socketTransport.

With the last approach the data is picked from the PRNetAddr structure and converted just once. But if adding another type and a member variable for the httpChannel is a really bad idea we go with holding a reference to the socketTransport.

Comment 19

[Honza Bambas (:mayhemer)]

Srirang, sorry not to mention, but I have almost ready patch. I was just loaded with 1.9.2 blocking stuff and some other personal matters that prevented me from finishing the patch up. I'll attach it after some cleanup if you agree.

Comment 20

[Srirang G Doddihal (Brahmana)]

(In reply to comment #19)
> Srirang, sorry not to mention, but I have almost ready patch. I was just loaded
> with 1.9.2 blocking stuff and some other personal matters that prevented me
> from finishing the patch up. I'll attach it after some cleanup if you agree.

That's totally fine. I picked this up just because this feature is really useful to me. Please do attach the patch when you have it ready.

I fully agree. :-)

Comment 21

[Honza Bambas (:mayhemer)]

(In reply to comment #18)
> To avoid having more members in nsHttpChannel (as it already has too many and
> it becomes hard to understand their interactions) three suggestions came up:
> 
> 1) Put a getter on nsHttpConnection for the socketTransport -- Not possible as
> the nsHttpConnection object should be accessed only on socket thread.
> 

I had the same idea, but between onStartRequest and onStopReuqest (including) we might not have the connection (transaction has already finished its job and released the connection).

> 2) Hold a reference to the nsISocketTransport in the channel. We get it in
> OnTransportStatus.
> 

I have to verify, but it could lead to left open connections, but I'm not sure. IMHO the last resort solution.

> 3) Combine the four attributes into a structure called
> nsHttpConnectionProperties and have an instance of that as a member variable on
> nsHttpChannel. This way we will not hold a reference to the socketTransport.
> 
> With the last approach the data is picked from the PRNetAddr structure and
> converted just once. But if adding another type and a member variable for the
> httpChannel is a really bad idea we go with holding a reference to the
> socketTransport.

Maybe hold both self and peer addresses? I vote for this approach personally. Going to attach the patch soon.

Stealing the bug back to me.

Comment 22

[Honza Bambas (:mayhemer)]

Attachment: v0.9

This is patch for consideration, w/ modified test_traceable_channel.js test to check we get the info in OnStartRequest. 

It's depending on bug 534698. 

I see one problem with this patch, we may get OnTransportStatus after the transaction has already released its connection (as we get it through a inter-thread proxy). Then the transport becomes inaccessible. I cannot QI the trans arg in nsHttpChannel::OnTransportStatus because http transaction doesn't take the transport reference in its OnTransportStatus method (own signature) and passes null as transport argument to the transport event sink.

bz, do you think that passing the transport through http transaction OnTransportStatus would be a good idea to make this change really work?

Comment 23

[Honza Bambas (:mayhemer)]

Attachment: v1

This is my final suggestion, I let nsITransport go through the transaction to sinks.  After bug 534698 has landed we can use STATUS_CONNECTED_TO to read data from the socket transport in http channel.

This patch seems to be safe, socket transport is not doing anything important in its destructor that might get delayed by this patch (transport is held for a moment by the sink proxy).  Transport seems to live much longer then its members (w/o the patch), so there is low risk of leaks or anything unexpected.  I didn't find any particular reason not to pass socket transport object through the transaction up, the API was introduced in bug 176919.

Comment 24

[carglue a t ya hoo do tcom]

This patch would only partially solve our needs unless the targeted IP and port could also be retrieved via the STATUS_CONNECTING_TO (as opposed to STATUS_CONNECTED_TO) and STATUS_SENDING_TO notifications.  Reason being, we need to report what IP and port are being requested upon connection failure, where we won't receive the STATUS_CONNECTED_TO notification.  Also, for keep-alives, we won't even get STATUS_CONNECTED_TO notification.  See comment 10.

Comment 25

[Honza Bambas (:mayhemer)]

(In reply to comment #24)
> This patch would only partially solve our needs unless the targeted IP and port
> could also be retrieved via the STATUS_CONNECTING_TO (as opposed to
> STATUS_CONNECTED_TO) and STATUS_SENDING_TO notifications.  Reason being, we
> need to report what IP and port are being requested upon connection failure,
> where we won't receive the STATUS_CONNECTED_TO notification.  

This isn't possible. Socket transport is designed (from a good reason) not to report the address sooner then after it is CONNECTED_TO (internally in STATE_TRANSFERRING).  The reason is that for a host name you can resolve more then a single IP, socket transport then tries to connect them all one by one.  After none of them could be connected it reports an error, but not any of IPs it has used.  You don't have a single address to report as faulty.  In that case you must take nsIChannel.uri.host and nsIChannel.uri.port ('undefault' it) and report the "host:port" pair.  For 'undefaulting' see [1], if the port is e.g. 80 for http, nsIURI.port will return -1, what means, that we have to use the schema default port (80 for http, 443 for https, 21 for ftp etc...)

> Also, for
> keep-alives, we won't even get STATUS_CONNECTED_TO notification.  See comment
> 10.

Good point, that is true. Will update the patch.

[1] http://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsNetUtil.h#318

Comment 26

[Honza Bambas (:mayhemer)]

Attachment: v1.1

Updated patch.  I am using WAITING_FOR to get the address as well, that is always guaranteed to be notified.  SENDING_TO is notified only when we are sending a request body (in most cases only for POSTs).

Comment 27

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Any progress on the review?
This would be great to have for Firebug.
Honza

Comment 28

[Matthew Gertner]

We're waiting for this as well.

Comment 29

[Zack Weinberg (:zwol)]

I'd very much like to have this as well.

Comment 30

[Zack Weinberg (:zwol)]

This patch seems to have substantial overlap with some of the infrastructure being added for bug 354493, I wanted to make folks aware of that.

Is there anything I can do to help get this one done?

Comment 31

[Jan Honza Odvarko [:Honza] (always need-info? me)]

mayhemer: any chance we can get this fixed?

Honza

Comment 32

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

Comment on attachment 424007 [details] [diff] [review]
v1.1

+++ b/netwerk/protocol/http/public/nsIHttpChannelInternal.idl

need a new IID

+++ b/netwerk/protocol/http/src/nsHttpChannel.cpp
+nsHttpChannel::GetServerAddress(nsACString & _result)
+{
+    _result.SetCapacity(64);
+    PR_NetAddrToString(&mPeerAddr, _result.BeginWriting(), 64);

Please check that mPeerAddr.raw.family != PR_AF_UNSPEC, like you do in GetServerPort

+nsHttpChannel::GetClientPort(PRInt32 * _result)
+{
+    NS_ENSURE_ARG_POINTER(_result);

for consistency with GetServerPort, please check for PR_AF_UNSPEC in both places or none.

Comment 33

[Zack Weinberg (:zwol)]

Attachment: v1.2

Stealing; I want to get this landed before FF5 branches.  I've merged it up, made Christian's requested changes, and fixed the English in a bunch of comments.

A problem has come up during the merge, unfortunately: there is now an HttpChannelChild class that also needs to provide implementations of the new nsIHttpChannelInternal methods, but I don't know if it needs to implement them for reals or if it can get away with always returning NS_ERROR_NOT_AVAILABLE.  (If you try to compile this patch as is you'll get a bunch of "cannot create instance of abstract class" errors.)

It also occurs to me that the attributes would be better named localAddress, localPort, peerAddress, peerPort.  Thoughts?

Comment 34

[Josh Matthews [:jdm]]

If there's any chance that an extension will want retrieve the values of the new attributes in a content process, we'll need to have some way of transferring them from the parent to the child.  The drop-dead simple way is to add a couple of synchronous IPDL getters, but it would probably be nicer to move the implementation and C++ members to BaseHttpChannel and simply forward the values from the parent to the child when they're first set in OnTransportStatus.

Comment 35

[Josh Matthews [:jdm]]

Presuming they're available by the time OnStartRequest is called, it would be a simple matter to add them to the mass of values being passed from parent to child there.  Of course, we'll need to serialize PRNetAddr by hand. Sigh.

Comment 36

[Zack Weinberg (:zwol)]

I don't know how to do any of the above (this would be my first ever time having to do something with the IPC layer), and with the timing being what it is, I'm rather inclined to just stub them in HttpChannelChild for now and file a new bug.  Also, aren't extensions by definition executing in the chrome process?

Comment 37

[Josh Matthews [:jdm]]

Filing a followup should be fine, IMO.  And extensions do operate in the chrome process, but they can run scripts in the content process which have access to necko channels.

Comment 38

[Zack Weinberg (:zwol)]

Attachment: v1.3

Working revision with stubs in HttpChannelChild (passed netwerk xpcshell tests locally; pushed to try).

Comment 39

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

Comment on attachment 524889 [details] [diff] [review]
v1.3

> It also occurs to me that the attributes would be better named localAddress,
> localPort, peerAddress, peerPort.  Thoughts?

I'm fine with either naming... I don't really have a preference

> Of course, we'll need to serialize PRNetAddr by hand. Sigh.

Nope, you could send it as 2 strings + 2 ints.

Comment 40

[Zack Weinberg (:zwol)]

http://hg.mozilla.org/mozilla-central/rev/8bf059c601c8

Filed bug 648878 on the stubs in HttpChannelChild.

Comment 41

[Eric Shepherd [:sheppy]]

Documented the new attributes:

https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIHttpChannelInternal

And mentioned on:

https://developer.mozilla.org/en/Firefox_5_for_developers

Comment 42

[Jan Honza Odvarko [:Honza] (always need-info? me)]

The documentation says:

This may throw an NS_ERROR_NOT_AVAILABLE exception if accessed when the channel's endpoints haven't been determined yet, or any time the nsIHttpActivityObserver.isActive attribute is false. See bug 534698 and bug 526207 .

What is the best moment to access the new fields (localAdderss, remoteAddres, etc.) For instance, I am using nsIActivityObserver.observeActivity, is any of its events preferred to get the info?

Honza

Comment 43

[Honza Bambas (:mayhemer)]

(In reply to comment #42)
> What is the best moment to access the new fields (localAdderss,
> remoteAddres, etc.) 

You can wait for state nsISocketTransport::STATUS_WAITING_FOR, this event is always delivered to activity observers for each channel being observed.  Then you can be sure both the local and remote IP address and port are available.  This definitely works in desktop Firefox but is probably broken in Fennec (bug 659251).

Comment 44

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Sometimes it seems to be too soon getting the info at nsISocketTransport::STATUS_WAITING_FOR, since I am seeing following exception:

[Exception... "Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsIHttpChannelInternal.remoteAddress]"  nsresult: "0x80040111 (NS_ERROR_NOT_AVAILABLE)"

Using nsIHttpActivityObserver.ACTIVITY_SUBTYPE_RESPONSE_COMPLETE works for me.

Honza

Comment 45

[Honza Bambas (:mayhemer)]

(In reply to Jan Honza Odvarko from comment #44)
> Sometimes it seems to be too soon getting the info at
> nsISocketTransport::STATUS_WAITING_FOR, since I am seeing following
> exception:
> 
> [Exception... "Component returned failure code: 0x80040111
> (NS_ERROR_NOT_AVAILABLE) [nsIHttpChannelInternal.remoteAddress]"  nsresult:
> "0x80040111 (NS_ERROR_NOT_AVAILABLE)"
> 
> Using nsIHttpActivityObserver.ACTIVITY_SUBTYPE_RESPONSE_COMPLETE works for
> me.
> 
> Honza


Problem is following:
- first, we post an event to distributor observers from the network thread (in nsHttpTransaction::OnTransportStatus)
- second, we post an event to the channel (again, in nsHttpTransaction::OnTransportStatus)
- if we reuse a connection for that channel, there is no STATUS_CONNECTED_TO sent to the channel
- therefor the channel grabs addresses from the transport in STATUS_WAITING_FOR event first
- this event is delivered to distributor observers sooner then to the channel -> when observers call on the channel it doesn't have the address yet

I'll file a bug for this, we may want to reverse order of posting.