Closed
Bug 546581
Opened 15 years ago
Closed 15 years ago
e10s HTTP: create common base class for HttpChannelChild and nsHttpChannel
Categories
(Core :: Networking: HTTP, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: jduell.mcbugs, Assigned: dwitte)
References
Details
Attachments
(2 files, 4 obsolete files)
(deleted),
patch
|
lusian
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jduell.mcbugs
:
review+
|
Details | Diff | Splinter Review |
I think we've hit the point where it's clear that there's a lot of common logic that we've been cut-and-pasting from nsHttpChannel to HttpChannelChild. Most of the functions are small, but some of them aren't (ex: Init()), so I think we should create a common base class for the two of them.
Notes on doing this:
Let's call the class nsHttpBaseChannel. (Just to be really confusing, there's already an unrelated "BaseChannel" in the netwerk tree, which is used by most of the channels except HTTP).
Put it in the mozilla::net namespace, and use 2-space indents (i.e. same formatting as HttpChannelChild, and not the 4-space indents in older necko code).
We can share the entire Init() function. This will mean changing the call signature of HttpChannelChild's to look like nsHttpChannel's. We'll also need the mCaps and mConnectionInfo variables in HttpChannelChild. Make sure to delete mConnectionInfo in all the same places that nsHttpChannel does (except obviously in codepaths that don't exist in HttpChannelChild).
Part of this patch should also modify nsHttpHandler::NewProxiedChannel so that most of the HttpChannelChild-specific logic is removed. We should do a simple switch based on IsNeckoChild, and NS_NEWXPCOM an HttpChannelChild if so, otherwise an nsHttpChannel, and from then on there should be one codepath that operates on an nsHttpBaseChannel. Note that this means nsHttpBaseChannel must be an nsIChannel.
Assignee | ||
Comment 1•15 years ago
|
||
Jason, do you plan to work on this? (Or Honza?) I could take a stab.
Comment 2•15 years ago
|
||
I probably won't, not sooner then in a week or so, having other stuff to do.
Reporter | ||
Comment 3•15 years ago
|
||
Also, for now let's use ENSURE_CALLED_BEFORE_ASYNC_OPEN() in the base class in the places where it's currently in HttpChannelChild. We may bump into places where that breaks things for nsHttpChannel, but it would be good to know if/were that happens. Hopefully there's not much code out there that tries to add headers, set GET/POST, etc., after AsyncOpen, as it's a meaningless semantic (but which nsHttpChannel hasn't explicitly marked as erroneous).
Assigning to Dan.
Assignee: nobody → dwitte
Reporter | ||
Comment 4•15 years ago
|
||
Nit: let's call the class HttpBaseChannel, not nsHttpBaseChannel, since we're otherwise "modernizing" it via mozilla:net and 2-space indents.
As I'm about to mention in bug 536279, we should move SetRequestHeader() into the base class, so HttpChannelChild can call it as part of its version of that function.
Comment 5•15 years ago
|
||
See bug #541017 comment #10 and comment #11 for a possible starting-point...
Assignee | ||
Comment 6•15 years ago
|
||
Pretty self-explanatory. Mostly code movement here. The salient bits are:
* Moves all the 'non-controversial' bits of nsHttpChannel into HttpBaseChannel. nsHttpChannel and HttpChannelChild override the more controversial (or implementation-dependent) methods with their own implementations that either DROP_DEAD() or do the right thing, resp.
* Transforms some 'if (!mIsPending)' checks from what used to be nsHttpChannel code into ENSURE_CALLED_BEFORE_ASYNC_OPEN(), which will now cause the non-e10s build (and e10s parent process) to DROP_DEAD() if violated.
* Adds serialization of mRequestHead.Method(), mRedirectionLimit, mAllowPipelining, and mForceAllowThirdPartyCookie to HttpChannelChild::AsyncOpen(). These seemed pretty non-controversial to me.
* HttpBaseChannel also includes (and inits) mConnectionInfo and mCaps. HttpChannelChild doesn't really do anything with them now, but I included a couple TODO's about possibly serializing them in future.
* This is on top of Jae-Seong's patch in bug 536279, and my cookie patch in bug 537156. (In particular, it uses serialization of nsHttpAtom from the former.)
This passes all non-e10s tests and e10s test_simple_wrap.js and test_head_wrap.js.
Attachment #429885 -
Flags: review?(jduell.mcbugs)
Assignee | ||
Comment 7•15 years ago
|
||
Adds a simple test for forceAllowThirdPartyCookie.
Attachment #430445 -
Flags: review?(jduell.mcbugs)
Assignee | ||
Comment 8•15 years ago
|
||
(In reply to comment #6)
> * Transforms some 'if (!mIsPending)' checks from what used to be nsHttpChannel
> code into ENSURE_CALLED_BEFORE_ASYNC_OPEN(), which will now cause the non-e10s
> build (and e10s parent process) to DROP_DEAD() if violated.
On second though, this is a bit heavy-handed. Maybe just have HttpBaseChannel return an error, and have HttpChannelChild override those functions with versions that DROP_DEAD()? Thoughts?
Reporter | ||
Comment 9•15 years ago
|
||
Have you run the patch through the xpcshell and mochitests, i.e. a tryserver build? If we're getting through all of those, then I think it's fine to DROP_DEAD for now in the base class. We've got to remove all of these DROP_DEADs before we ship anyway, and if we hit them via nsHttpChannel, it's be good to know what's going on. But we don't want to commit this untested, as it could break the non-experimental, one-necko-stack-per-process model that it currently the default.
Reporter | ||
Updated•15 years ago
|
Attachment #429885 -
Flags: review?(jduell.mcbugs) → review?(lusian)
Reporter | ||
Comment 10•15 years ago
|
||
Comment on attachment 429885 [details] [diff] [review]
patch v1
Jae-Seong will do a first review; assign to me for a 2nd review when you're done. Thanks!
Reporter | ||
Updated•15 years ago
|
Attachment #430445 -
Flags: review?(jduell.mcbugs) → review?(lusian)
Comment 11•15 years ago
|
||
Comment on attachment 429885 [details] [diff] [review]
patch v1
Dan, please update the patch. It fails to apply.
Assignee | ||
Comment 12•15 years ago
|
||
Updated.
Attachment #429885 -
Attachment is obsolete: true
Attachment #435013 -
Flags: review?
Attachment #429885 -
Flags: review?(lusian)
Assignee | ||
Updated•15 years ago
|
Attachment #435013 -
Flags: review? → review?(lusian)
Updated•15 years ago
|
Attachment #430445 -
Flags: review?(lusian) → review+
Updated•15 years ago
|
Attachment #430445 -
Flags: review?(jduell.mcbugs)
Comment 13•15 years ago
|
||
Comment on attachment 435013 [details] [diff] [review]
patch v2
Should we follow prefix conventions and change "value" to "aValue", or is it okay to leave them unchanged?
>+HttpBaseChannel::GetAllowPipelining(PRBool *value)
>+HttpBaseChannel::SetAllowPipelining(PRBool value)
>+HttpBaseChannel::GetRedirectionLimit(PRUint32 *value)
>+HttpBaseChannel::SetRedirectionLimit(PRUint32 value)
>+HttpBaseChannel::IsNoStoreResponse(PRBool *value)
>+HttpBaseChannel::IsNoCacheResponse(PRBool *value)
etc
Dan, sorry to keep bugging you, but please update HttpChannelChild.cpp & HttpChannelChild.h. The patch fails because of mForceAllowThirdPartyCookie, a missing header file. Other files patch cleanly. Just the two files, please.
Comment 14•15 years ago
|
||
most newer necko code doesn't use the "a" prefix
Assignee | ||
Comment 15•15 years ago
|
||
Sorry -- I forgot I had a patch earlier in my queue that this depended on. :(
This version contains both patches, and should apply to e10s trunk.
Attachment #435013 -
Attachment is obsolete: true
Attachment #435318 -
Flags: review?(lusian)
Attachment #435013 -
Flags: review?(lusian)
Reporter | ||
Comment 16•15 years ago
|
||
So from running the last patch through tryserver, it turns out we were missing a header file, and that SetLoadFlags can't use ENSURE_CALLED_BEFORE_ASYNC_OPEN (load flags are set/gotten after asyncOpen, but are only used on the tab side as near as I can tell, so we don't have to propagate them. I don't like the hack, but am fine with containing this sort of this to just the LoadFlags).
I've also rolled the test into the main patch--in general I'd prefer we do that--otherwise we're likely to forget to check in tests.
Attachment #430445 -
Attachment is obsolete: true
Attachment #435318 -
Attachment is obsolete: true
Attachment #435540 -
Flags: review?(lusian)
Attachment #430445 -
Flags: review?(jduell.mcbugs)
Attachment #435318 -
Flags: review?(lusian)
Updated•15 years ago
|
Attachment #435540 -
Flags: review?(lusian)
Attachment #435540 -
Flags: review?(jduell.mcbugs)
Attachment #435540 -
Flags: review+
Comment 17•15 years ago
|
||
Comment on attachment 435540 [details] [diff] [review]
Fixes for tryserver, and roll test code into main patch
>+HttpBaseChannel::~HttpBaseChannel()
>+{
>+ LOG(("Destroying HttpBaseChannel @%x\n", this));
>+
>+ // release our reference to the handler
>+ nsHttpHandler* handler = gHttpHandler;
>+ NS_RELEASE(handler);
>+}
Can't we just NS_RELEASE(gHttpHandler), which Jason did in http://mxr.mozilla.org/projects-central/source/electrolysis/netwerk/protocol/http/src/HttpChannelChild.cpp#83 ?
Reporter | ||
Comment 18•15 years ago
|
||
Hmm... On OSX I'm seeing a different error:
HttpBaseChannel.cpp:633: error: 'mResponseHead' was not declared in this scope
What's weird about this is that mResponseHead is used plenty in the file before this point, but after it all refs seem to generate an error.
We of course don't care about OSX too much right now, but our code has at least been compiling, so it'd be nice to figure out what this is about.
Assignee | ||
Comment 19•15 years ago
|
||
(In reply to comment #17)
> Can't we just NS_RELEASE(gHttpHandler), which Jason did in
> http://mxr.mozilla.org/projects-central/source/electrolysis/netwerk/protocol/http/src/HttpChannelChild.cpp#83
> ?
No. That would null out gHttpHandler, which would be bad times.
Assignee | ||
Comment 20•15 years ago
|
||
(In reply to comment #18)
> HttpBaseChannel.cpp:633: error: 'mResponseHead' was not declared in this scope
>
> What's weird about this is that mResponseHead is used plenty in the file before
> this point, but after it all refs seem to generate an error.
All refs, or just refs where 'mResponseHead' is used in a 'return' statement?
It's an nsAutoPtr, which is templated; perhaps gcc on OSX has buggy template handling.
In
+ return mResponseHead->GetHeader(atom, value);
try putting the 'mResponseHead->GetHeader(atom, value)' on a separate line, and then 'return rv'. Maybe that'll work!
Assignee | ||
Updated•15 years ago
|
Attachment #436629 -
Flags: review? → review?(jduell.mcbugs)
Reporter | ||
Comment 22•15 years ago
|
||
http://hg.mozilla.org/projects/electrolysis/rev/f5972978cb6e
Modified patch so that no empty methods exist in HttpBaseChannel.
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 23•15 years ago
|
||
Comment on attachment 436629 [details] [diff] [review]
mac fixes
clearing review on old patch. Logic made it into the patch that landed.
Attachment #436629 -
Flags: review?(jduell.mcbugs) → review+
Reporter | ||
Comment 24•15 years ago
|
||
Comment on attachment 435540 [details] [diff] [review]
Fixes for tryserver, and roll test code into main patch
removing review from obsolete patch
Attachment #435540 -
Flags: review?(jduell.mcbugs)
You need to log in
before you can comment on or make changes to this bug.
Description
•