Closed
Bug 648878
Opened 13 years ago
Closed 13 years ago
Implement Get(Local|Remote)(Address|Port) in HttpChannelChild
Categories
(Core :: Networking: HTTP, defect)
Tracking
()
RESOLVED
FIXED
mozilla6
People
(Reporter: zwol, Assigned: u408661)
References
Details
Attachments
(2 files, 3 obsolete files)
23.39 KB,
patch
|
u408661
:
review+
|
Details | Diff | Splinter Review |
5.32 KB,
patch
|
jdm
:
review+
|
Details | Diff | Splinter Review |
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•13 years ago
|
||
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.
Whiteboard: [good first bug]
Updated•13 years ago
|
Keywords: student-project
Attachment #526744 -
Flags: review?(jduell.mcbugs)
Comment 3•13 years ago
|
||
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.
Attachment #526744 -
Flags: review?(jduell.mcbugs) → review-
(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•13 years ago
|
||
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.
New and improved, now with 100% less friend and ipdl message adding!
Attachment #526744 -
Attachment is obsolete: true
Attachment #528871 -
Flags: review?(jduell.mcbugs)
Updated•13 years ago
|
Attachment #528871 -
Flags: review?(jduell.mcbugs) → review+
Updated•13 years ago
|
Keywords: student-project → checkin-needed
Whiteboard: [good first bug]
Comment 7•13 years ago
|
||
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.
Keywords: checkin-needed
Let's try this again... now with fixes for windows building!
Attachment #528871 -
Attachment is obsolete: true
Attachment #530050 -
Flags: review?(jduell.mcbugs)
Comment 9•13 years ago
|
||
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.
Attachment #530050 -
Flags: review?(jduell.mcbugs) → review+
Assignee | ||
Comment 10•13 years ago
|
||
Fixes whitespace. Carry forward r+
Attachment #530050 -
Attachment is obsolete: true
Attachment #530174 -
Flags: review+
Comment 12•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/adf273dbca38
Status: NEW → RESOLVED
Closed: 13 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla6
Updated•13 years ago
|
Keywords: checkin-needed
Comment 13•13 years ago
|
||
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)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 14•13 years ago
|
||
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.
Depends on: 659251
Comment 15•13 years ago
|
||
This patch also didn't remove the stubs from HttpChannelChild that all return NS_ERROR_NOT_AVAILABLE.
Comment 16•13 years ago
|
||
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).
Attachment #539125 -
Flags: review?(josh)
Comment 17•13 years ago
|
||
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•13 years ago
|
||
Comment on attachment 539125 [details] [diff] [review] Fix getlocal functions Review of attachment 539125 [details] [diff] [review]: -----------------------------------------------------------------
Attachment #539125 -
Flags: review?(josh) → review+
Comment 19•13 years ago
|
||
Try seems happy. Landing fixup as bug 664163. Since original patch in this bug has stuck, marking FIXED.
Status: REOPENED → RESOLVED
Closed: 13 years ago → 13 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•