Closed Bug 546581 Opened 14 years ago Closed 14 years ago

e10s HTTP: create common base class for HttpChannelChild and nsHttpChannel

Categories

(Core :: Networking: HTTP, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: jduell.mcbugs, Assigned: dwitte)

References

Details

Attachments

(2 files, 4 obsolete files)

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.
Blocks: fennecko
Jason, do you plan to work on this? (Or Honza?) I could take a stab.
I probably won't, not sooner then in a week or so, having other stuff to do.
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
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.
See bug #541017 comment #10 and comment #11 for a possible starting-point...
Blocks: 548275
Attached patch patch v1 (obsolete) — Splinter Review
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)
Attached patch thirdparty cookie test (obsolete) — Splinter Review
Adds a simple test for forceAllowThirdPartyCookie.
Attachment #430445 - Flags: review?(jduell.mcbugs)
(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?
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.
Attachment #429885 - Flags: review?(jduell.mcbugs) → review?(lusian)
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!
Attachment #430445 - Flags: review?(jduell.mcbugs) → review?(lusian)
Comment on attachment 429885 [details] [diff] [review]
patch v1

Dan, please update the patch.  It fails to apply.
Attached patch patch v2 (obsolete) — Splinter Review
Updated.
Attachment #429885 - Attachment is obsolete: true
Attachment #435013 - Flags: review?
Attachment #429885 - Flags: review?(lusian)
Attachment #435013 - Flags: review? → review?(lusian)
Attachment #430445 - Flags: review?(lusian) → review+
Attachment #430445 - Flags: review?(jduell.mcbugs)
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.
most newer necko code doesn't use the "a" prefix
Attached patch patch v3 (obsolete) — Splinter Review
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)
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)
Attachment #435540 - Flags: review?(lusian)
Attachment #435540 - Flags: review?(jduell.mcbugs)
Attachment #435540 - Flags: review+
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 ?
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.
(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.
(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!
Attached patch mac fixesSplinter Review
Fixes the mac build for me.
Attachment #436629 - Flags: review?
Attachment #436629 - Flags: review? → review?(jduell.mcbugs)
http://hg.mozilla.org/projects/electrolysis/rev/f5972978cb6e

Modified patch so that no empty methods exist in HttpBaseChannel.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
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+
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.

Attachment

General

Created:
Updated:
Size: