Add IP address and port info to nsIHttpActivityObserver

RESOLVED FIXED

Status

()

Core
Networking: HTTP
--
enhancement
RESOLVED FIXED
8 years ago
6 years ago

People

(Reporter: Matthew Gertner, Assigned: zwol)

Tracking

(Depends on: 1 bug, {dev-doc-complete})

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [firebug-p2])

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

8 years ago
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.
Yes, this would be great to have for Firebug.
Honza
Whiteboard: [firebug-p2]
Created attachment 410886 [details] [diff] [review]
nsSocketTransport2.cpp patch for passing IP and port via notification
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.  =)
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
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?
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 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?
Attachment #410886 - Flags: review-
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.
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)
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.
(Reporter)

Comment 11

8 years ago
(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. :-)
(Reporter)

Comment 12

8 years ago
(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?
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.
(Reporter)

Comment 14

8 years ago
(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?
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.
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.
Assignee: honzab.moz → om.brahmana
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.
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.
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.
(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. :-)
(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.
Assignee: om.brahmana → honzab.moz
Depends on: 534698
Created attachment 417555 [details] [diff] [review]
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?
Created attachment 423837 [details] [diff] [review]
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.
Attachment #410886 - Attachment is obsolete: true
Attachment #417555 - Attachment is obsolete: true
Attachment #423837 - Flags: review?(cbiesinger)
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.
(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
Created attachment 424007 [details] [diff] [review]
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).
Attachment #423837 - Attachment is obsolete: true
Attachment #424007 - Flags: review?(cbiesinger)
Attachment #423837 - Flags: review?(cbiesinger)
Any progress on the review?
This would be great to have for Firebug.
Honza
(Reporter)

Comment 28

7 years ago
We're waiting for this as well.
(Assignee)

Comment 29

7 years ago
I'd very much like to have this as well.
(Assignee)

Comment 30

7 years ago
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?
mayhemer: any chance we can get this fixed?

Honza
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.
Attachment #424007 - Flags: review?(cbiesinger) → review+
(Assignee)

Comment 33

6 years ago
Created attachment 524861 [details] [diff] [review]
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?
Assignee: honzab.moz → zackw
Attachment #524861 - Flags: review?(cbiesinger)
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.
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.
(Assignee)

Comment 36

6 years ago
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?
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.
(Assignee)

Comment 38

6 years ago
Created attachment 524889 [details] [diff] [review]
v1.3

Working revision with stubs in HttpChannelChild (passed netwerk xpcshell tests locally; pushed to try).
Attachment #424007 - Attachment is obsolete: true
Attachment #524861 - Attachment is obsolete: true
Attachment #524889 - Flags: review?(cbiesinger)
Attachment #524861 - Flags: review?(cbiesinger)
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.
Attachment #524889 - Flags: review?(cbiesinger) → review+
(Assignee)

Updated

6 years ago
Blocks: 648878
(Assignee)

Comment 40

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

Filed bug 648878 on the stubs in HttpChannelChild.
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Assignee)

Updated

6 years ago
Keywords: dev-doc-needed
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
Keywords: dev-doc-needed → dev-doc-complete

Updated

6 years ago
Depends on: 650116
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
(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).
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
(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.
Depends on: 687456
You need to log in before you can comment on or make changes to this bug.