e10s HTTP: create common base class for HttpChannelChild and nsHttpChannel

RESOLVED FIXED

Status

()

Core
Networking: HTTP
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: jduell, Assigned: dwitte@gmail.com)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

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.
(Reporter)

Updated

8 years ago
Blocks: 516730
(Assignee)

Comment 1

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

Comment 3

8 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

8 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

8 years ago
See bug #541017 comment #10 and comment #11 for a possible starting-point...

Updated

8 years ago
Blocks: 548275
(Assignee)

Comment 6

8 years ago
Created attachment 429885 [details] [diff] [review]
patch v1

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

8 years ago
Created attachment 430445 [details] [diff] [review]
thirdparty cookie test

Adds a simple test for forceAllowThirdPartyCookie.
Attachment #430445 - Flags: review?(jduell.mcbugs)
(Assignee)

Comment 8

8 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

8 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

8 years ago
Attachment #429885 - Flags: review?(jduell.mcbugs) → review?(lusian)
(Reporter)

Comment 10

8 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

8 years ago
Attachment #430445 - Flags: review?(jduell.mcbugs) → review?(lusian)

Comment 11

8 years ago
Comment on attachment 429885 [details] [diff] [review]
patch v1

Dan, please update the patch.  It fails to apply.
(Assignee)

Comment 12

8 years ago
Created attachment 435013 [details] [diff] [review]
patch v2

Updated.
Attachment #429885 - Attachment is obsolete: true
Attachment #435013 - Flags: review?
Attachment #429885 - Flags: review?(lusian)
(Assignee)

Updated

8 years ago
Attachment #435013 - Flags: review? → review?(lusian)

Updated

8 years ago
Attachment #430445 - Flags: review?(lusian) → review+

Updated

8 years ago
Attachment #430445 - Flags: review?(jduell.mcbugs)

Comment 13

8 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.
most newer necko code doesn't use the "a" prefix
(Assignee)

Comment 15

8 years ago
Created attachment 435318 [details] [diff] [review]
patch v3

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

8 years ago
Created attachment 435540 [details] [diff] [review]
Fixes for tryserver, and roll test code into main patch

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

8 years ago
Attachment #435540 - Flags: review?(lusian)
Attachment #435540 - Flags: review?(jduell.mcbugs)
Attachment #435540 - Flags: review+

Comment 17

8 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

8 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

8 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

8 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)

Comment 21

8 years ago
Created attachment 436629 [details] [diff] [review]
mac fixes

Fixes the mac build for me.
Attachment #436629 - Flags: review?
(Assignee)

Updated

8 years ago
Attachment #436629 - Flags: review? → review?(jduell.mcbugs)
(Reporter)

Comment 22

8 years ago
http://hg.mozilla.org/projects/electrolysis/rev/f5972978cb6e

Modified patch so that no empty methods exist in HttpBaseChannel.
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
(Reporter)

Comment 23

8 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

8 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.