Last Comment Bug 648878 - Implement Get(Local|Remote)(Address|Port) in HttpChannelChild
: Implement Get(Local|Remote)(Address|Port) in HttpChannelChild
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Networking: HTTP (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: mozilla6
Assigned To: Nicholas Hurley [:nwgh][:hurley]
:
Mentors:
Depends on: 526207 659251
Blocks: 664163
  Show dependency treegraph
 
Reported: 2011-04-10 10:38 PDT by Zack Weinberg (:zwol)
Modified: 2012-03-06 02:26 PST (History)
10 users (show)
bzbarsky: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch v1 (16.95 KB, patch)
2011-04-18 09:06 PDT, Nicholas Hurley [:nwgh][:hurley]
jduell.mcbugs: review-
Details | Diff | Review
patch v2 (23.36 KB, patch)
2011-04-28 09:15 PDT, Nicholas Hurley [:nwgh][:hurley]
jduell.mcbugs: review+
Details | Diff | Review
patch v3 (23.47 KB, patch)
2011-05-04 09:54 PDT, Nicholas Hurley [:nwgh][:hurley]
jduell.mcbugs: review+
Details | Diff | Review
patch v3.1 (23.39 KB, patch)
2011-05-04 15:23 PDT, Nicholas Hurley [:nwgh][:hurley]
hurley: review+
Details | Diff | Review
Fix getlocal functions (5.32 KB, patch)
2011-06-13 22:54 PDT, Jason Duell [:jduell] (needinfo? me)
josh: review+
Details | Diff | Review

Description Zack Weinberg (:zwol) 2011-04-10 10:38:59 PDT
From bug 526207 comment 33 et seq.:

(Zack Weinberg)
> 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.)

(Josh Matthews)
> 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.

(Zack Weinberg)
> I don't know how to do any of the above (this would be my first time
> ever having to do something with the IPC layer), and with the schedule
> being what it is, I'm rather inclined to just stub them in
> HttpChannelChild for now and file a new bug.

This is that new bug.
Comment 1 Josh Matthews [:jdm] 2011-04-10 10:45:52 PDT
This would be a really good, well-scoped bug for anyone interested in learning how cross-process Firefox works.  I think the best way to implement the required functions is to move nsHttpChannel::Get(Local|Remote)(Address|Port) and nsHttpChannel::m(Self|Peer)Addr to BaseHttpChannel so both parent and child channels display the same behaviour.  Then in HttpChannelParent::OnStartRequest, we need to serialize mSelfAddr and mPeerAddr and send them to the child (netwerk/protocol/http/PHttpChannel.ipdl will require modification).  If anyone's picking this up, feel free to contact me for further help/advice.
Comment 2 Nicholas Hurley [:nwgh][:hurley] 2011-04-18 09:06:16 PDT
Created attachment 526744 [details] [diff] [review]
patch v1
Comment 3 Jason Duell [:jduell] (needinfo? me) 2011-04-23 18:53:13 PDT
Comment on attachment 526744 [details] [diff] [review]
patch v1

This will be +r with the following issues addressed:

Fewer, bigger IPDL messages are better than multiple smaller ones (each message could cause a context switch and other messaging overheads--see bug 561694 for example).  So let's get rid of the UpdateAddresses IPDL message, and send its data as part of OnStartRequest.  Unless there's some reason we shouldn't (but I don't see one).

>   friend class HttpChannelParent;

Why do we need the friend relationship with HttpBaseChannel?

>    /* We've been tricked by some socket family we don't know about! */

+1 for excellent comment.
Comment 4 Nicholas Hurley [:nwgh][:hurley] 2011-04-25 10:35:24 PDT
(In reply to comment #3)
> Comment on attachment 526744 [details] [diff] [review]
> patch v1
> 
> This will be +r with the following issues addressed:
> 
> Fewer, bigger IPDL messages are better than multiple smaller ones (each message
> could cause a context switch and other messaging overheads--see bug 561694 for
> example).  So let's get rid of the UpdateAddresses IPDL message, and send its
> data as part of OnStartRequest.  Unless there's some reason we shouldn't (but I
> don't see one).

That's easy enough to do. Consider it done once we hash out what's below.

> >   friend class HttpChannelParent;
> 
> Why do we need the friend relationship with HttpBaseChannel?

HttpChannelParent doesn't derive from HttpBaseChannel, so to be able to get the data to send to the child, I needed to get access somehow to those members in the base channel. I didn't see any methods that gave me that information, so it was either this, or make HttpChannelParent inherit from HttpBaseChannel, and have a bunch of unimplemented methods. I chose the former, but will happily update to do the latter (or something else, if you prefer).
Comment 5 Jason Duell [:jduell] (needinfo? me) 2011-04-25 23:16:40 PDT
There's a third way: add some 'public' (but not XPCOM) methods, GetSelfAddr and GetPeerAddr (might as well have them return references, for efficiency).   Public, non-XPCOM methods can only be accessed by other necko files (not plugins, etc).  But that's perfect for this use.  (There's a section in nsHttpChannel.h of other such methods that's marked "for internal necko use only": put them there.)

I prefer this to the friend def (or god forbid, inheritance), because it keeps more encapsulation.  So far we've managed to make e10s necko really look like a 'client' of original necko, and I'd like to keep it that way.
Comment 6 Nicholas Hurley [:nwgh][:hurley] 2011-04-28 09:15:56 PDT
Created attachment 528871 [details] [diff] [review]
patch v2

New and improved, now with 100% less friend and ipdl message adding!
Comment 7 Boris Zbarsky [:bz] 2011-05-03 16:30:52 PDT
I tried pushing this to cedar, but it doesn't compile on Windows, most obviously because:

  'local' : is not a member of 'PRNetAddr'

so I backed it out.
Comment 8 Nicholas Hurley [:nwgh][:hurley] 2011-05-04 09:54:11 PDT
Created attachment 530050 [details] [diff] [review]
patch v3

Let's try this again... now with fixes for windows building!
Comment 9 Jason Duell [:jduell] (needinfo? me) 2011-05-04 13:28:14 PDT
Comment on attachment 530050 [details] [diff] [review]
patch v3

Pushing to try (sorry Boris, I did that for the last patch too but didn't get the results before you pushed to cedar: this time I'll wait for try results before marking checkin-needed).

The code to handle the AF_LOCAL and AF_UNSPEC addresses isn't really needed (we don't ship around Unix domain sockets or raw sockets in the necko code), but does no harm, so that's fine.

One nit: please throw up a new version of the patch that converts the code you moved to HttpBaseChannel to 2 space indenting.
Comment 10 Nicholas Hurley [:nwgh][:hurley] 2011-05-04 15:23:40 PDT
Created attachment 530174 [details] [diff] [review]
patch v3.1

Fixes whitespace. Carry forward r+
Comment 11 Jason Duell [:jduell] (needinfo? me) 2011-05-04 20:41:18 PDT
try is green.
Comment 12 Boris Zbarsky [:bz] 2011-05-05 13:33:38 PDT
http://hg.mozilla.org/mozilla-central/rev/adf273dbca38
Comment 13 Jason Duell [:jduell] (needinfo? me) 2011-05-24 00:19:04 PDT
Ugh--how did we land this w/o a test?  There wasn't any code in our unit_ipc directory that used these AFAICT.   I discovered that these aren't working while checking in a unit_ipc test for bug 646373.

To reproduce:  

Reenable the request.localAddress, etc, accesses in netwerk/unit/test_traceable_channel.js  (I've commented them out for now to get that bug landed)

run "make check-one SOLO_FILE=test_traceable_channel_wrap.js"

The test fails (with very unhelpful error msg)
Comment 14 Jason Duell [:jduell] (needinfo? me) 2011-05-24 03:14:54 PDT
Part of the issue is that the logic that sets the HttpChannelChild's mSelfAddr/mPeerAddr is done *after* calling listener->OnStartRequest.   It needs to be before that call so OnStartReq can access the fields.

But there's something else weird going on:  I can't seem to access most nsIHttpChannelInternal fields from xpcshell. See bug 659251.
Comment 15 Josh Matthews [:jdm] 2011-06-09 12:40:31 PDT
This patch also didn't remove the stubs from HttpChannelChild that all return NS_ERROR_NOT_AVAILABLE.
Comment 16 Jason Duell [:jduell] (needinfo? me) 2011-06-13 22:54:13 PDT
Created attachment 539125 [details] [diff] [review]
Fix getlocal functions

Removed masking stub function from child, moves mSelf|Peer init to before client OnStart is called.  Add a check for SetPriority too, for good measure (not tested by any other xpcshell tests).
Comment 17 Jason Duell [:jduell] (needinfo? me) 2011-06-13 23:19:12 PDT
Hmm, curious as to whether the assumptions in the test that remoteHost == 127.0.0.1 are going to wash on mobile xpcshell testing.  Pushed to try to find out.

 http://tbpl.mozilla.org/?tree=Try&pusher=jduell@mozilla.com&rev=36d9608635ea
Comment 18 Josh Matthews [:jdm] 2011-06-13 23:28:49 PDT
Comment on attachment 539125 [details] [diff] [review]
Fix getlocal functions

Review of attachment 539125 [details] [diff] [review]:
-----------------------------------------------------------------
Comment 19 Jason Duell [:jduell] (needinfo? me) 2011-06-14 09:16:39 PDT
Try seems happy.

Landing fixup as bug 664163.  Since original patch in this bug has stuck, marking FIXED.
Comment 20 Honza Bambas (:mayhemer) 2012-03-02 10:53:58 PST
*** Bug 687456 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.