530952 - Get very simple HTTP requests working under e10s

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Jason Duell]

Attachment: Support simple HTTP requests under e10s

The attached patch is a cleaned-up snapshot of my e10s HTTP work, which is now at a state where very simple HTTP requests are working (under xpcshell only).  I'd like to get this into the e10s tree (pref'd off) so that potential collaborators on e10s necko can build on it (and so my patches don't get too huge).

The patch is of course for http://hg.mozilla.org/projects/electrolysis

In a nutshell:  I add a new pref, 'network.ipc.http.enabled', which if set causes necko in the child to hand out an HttpChannelChild instead of a nsHttpChannel.   The HttpChannelChild is the child half of an IPDL PHttpChannel protocol.  At AsyncOpen time it creates a matching actor on the parent, which in turn creates a regular nsHttpChannel to service the request.  As the HttpChannelParent receives the OnStartMsg, OnDataAvailable, and OnStopRequest events from the regular nsHttpChannel, it sends IPDL msgs to the child, which calls the methods on the original listener.

Right now the code is *very* simple.  I am not sending the full RequestHead or ResponseHead, only Content-Length and the body of the HTTP request, so code that does anything other than simply read the body of the request is not supported.  Load Groups, Redirects, Event listeners, etc. are also not supported.  Nor is Canceling requests.  The code currently leaks the HttpChannelParent/Child objects too :)

My coding strategy so far is to have the HttpChannelChild advertise the same set of XPCOM interfaces as nsHttpChannel, with dummy methods that simply drop dead when they are called.  I've been implementing methods in the order they've been hit by my xpcshell test;  at some point we'll be able to remove interfaces that we're not going to need to support in the child.  So there's a lot of skeleton code here...

To run my xpcshell test:  apply the patch for bug 521922, then this patch, and you should be able to set SOLO_FILE=test_simple_wrap.js and run "make check-one"

Comment 1

[Benjamin Smedberg]

Comment on attachment 414407 [details] [diff] [review]
Support simple HTTP requests under e10s

>diff --git a/browser/app/profile/firefox.js b/browser/app/profile/firefox.js

> pref("dom.ipc.plugins.enabled", false);
> pref("dom.ipc.tabs.enabled", false);
>+pref("network.ipc.http.enabled", false);

I don't think prefs are the right vehicle for this. Until it "mostly works" I think we should just use an environment-variable check.


>+LOCAL_INCLUDES=-I$(srcdir)/../protocol/http/src -I$(srcdir)/../base/src 

make this a list:

LOCAL_INCLUDES += \
  -I$(srcdir)/../... \
  $(NULL)

>diff --git a/netwerk/ipc/NeckoChild.cpp b/netwerk/ipc/NeckoChild.cpp

>-  if (XRE_GetProcessType() == GeckoProcessType_Content) {
>+  NS_ABORT_IF_FALSE(IsNeckoChild(), "InitNeckoChild called by non-child!");
>+
>+  if (!gNeckoChild) {
>     mozilla::dom::ContentProcessChild * cpc = 
>-      mozilla::dom::ContentProcessChild::GetSingleton();
>+        mozilla::dom::ContentProcessChild::GetSingleton();

Why reindent this? It seems correct the first time.

>+void NeckoChild::DestroyNeckoChild()

I don't think we ever need to explicitly destroy this actor: it will go away all by itself when the toplevel PContentProcess shuts down.

Also as noted on IRC, I wonder if we shouldn't skip the PNecko protocol altogether and just put whatever toplevel messages you need directly on PContentProcess. That doesn't matter to me much, though.

>diff --git a/netwerk/ipc/NeckoCommon.h b/netwerk/ipc/NeckoCommon.h

>+static inline PRBool 
>+IsNeckoChild() 
>+{
>+  return XRE_GetProcessType() == GeckoProcessType_Content;        
>+}

I don't think this should be labelled 'static', just 'inline'.

>diff --git a/netwerk/ipc/NeckoParent.cpp b/netwerk/ipc/NeckoParent.cpp

>+/*  Do we need Request headers on child?  Probably

Yes, almost certainly, at least because XMLHttpRequest can set custom headers. But it's probably not necessary just for getting test-ipc.xul working!

>+  NS_INTERFACE_MAP_ENTRY(nsICachingChannel)

It would be really interesting to figure out whether/how we plan to implement this, since cache tokens naturally live in the chrome process.

>+  NS_INTERFACE_MAP_ENTRY(nsICacheListener)

I'm pretty sure this is an implementation detail and can/should be removed.

>+  NS_INTERFACE_MAP_ENTRY(nsITransportEventSink)
>+  NS_INTERFACE_MAP_ENTRY(nsIProtocolProxyCallback)
>+  NS_INTERFACE_MAP_ENTRY(nsIProxiedChannel)

Also implementation details, I think.

>+  NS_INTERFACE_MAP_ENTRY(nsITraceableChannel)

Ugh, we'll have to figure out how debugging is going to work.

>+  NS_INTERFACE_MAP_ENTRY(nsIAuthPromptCallback)

Also an implementation detail.

>+bool 
>+HttpChannelChild::RecvOnStartRequest(const PRInt32& HACK_Content_length)
>+{
>+  LOG(("HttpChannelChild::RecvOnStartRequest [this=%x]\n", this));
>+
>+  mContentLength_HACK = HACK_Content_length;
>+  nsresult rv = mChildListener->OnStartRequest(this, mChildListenerContext);
>+  if (!NS_SUCCEEDED(rv)) {
>+    return false;  // kills child?  TODO:  what to do here?  Cancel request?

What is the in-process behavior if OnStartRequest fails? returning false is definitely the wrong solution.

>+bool 
>+HttpChannelChild::RecvOnDataAvailable(const nsCString& data,
>+                                      const PRUint32& offset,
>+                                      const PRUint32& count)
>+{

>+  /*
>+   * TODO: stop using nsCString (will it handle null bytes?). Use nsTArray<char>
>+   * instead?

I really don't think nsTArray is right, and nsCString is a good buffer for us (it handles null bytes just fine!)

>+   * - Also avoid copying data if possible.  

Yes. We need a better stringstream which can wrap this string temporarily and become invalid outside of this stack frame.

>+  rv = mChildListener->OnDataAvailable(this, mChildListenerContext,
>+                                       stringStream, offset, count);
>+  if (!NS_SUCCEEDED(rv)) {
>+    return false;  // kills child?  TODO:  what to do here?  Cancel request?

Documented behavior:

68      * An exception thrown from onDataAvailable has the side-effect of
69      * causing the request to be canceled.

>+/* void cancel (in nsresult aStatus); */
>+NS_IMETHODIMP
>+HttpChannelChild::Cancel(nsresult aStatus)
>+{
>+  DROP_DEAD();

We're going to need this one pretty quickly, if only to handle error conditions in the nsIStreamListener methods we call above.

>+HttpChannelChild::GetOriginalURI(nsIURI * *aOriginalURI)
>+HttpChannelChild::SetOriginalURI(nsIURI * aOriginalURI)
>+HttpChannelChild::GetURI(nsIURI * *aURI)

Seems like these will be pretty important.

>+HttpChannelChild::GetOwner(nsISupports * *aOwner)
>+HttpChannelChild::SetOwner(nsISupports * aOwner)

Ah, the lovely and completely underdocumented owner.

>+HttpChannelChild::GetNotificationCallbacks(nsIInterfaceRequestor * *aNotificationCallbacks)
>+HttpChannelChild::SetNotificationCallbacks(nsIInterfaceRequestor * aNotificationCallbacks)

We should probably support setting/getting this, even if we don't start using it immediately.

>+HttpChannelChild::GetContentType(nsACString & aContentType)
>+HttpChannelChild::SetContentType(const nsACString & aContentType)

Don't we already know this from the messages you have? This will be essential for proper browser functioning.

>+/* nsIInputStream open (); */
>+NS_IMETHODIMP
>+HttpChannelChild::Open(nsIInputStream **_retval NS_OUTPARAM)
>+{

This should probably just be return NS_ERROR_NOT_IMPLEMENTED, since we don't actually plan on implementing synchronous open.

>+   * - But does this mean we've got to keep child's nsIOSvs and HttpHandler
>+   *   port blacklists in sync with chrome's?

Yes, but since they are the same binary, we should just have a helper method shared between the implementations.

>+  // TODO: Can we combine constructor and AsyncOpen to save one IPC round-trip?  

Yes

>+  if (!gNeckoChild->SendPHttpChannelConstructor(this)) {
>+    // TODO: currently means "this" has been deleted! bug 529693
>+    DROP_DEAD();
>+  }
>+  // This corresponds to Release() in DeallocPHttpChannel 
>+  this->AddRef();

I think the AddRef should be *before* you call SendP, so that failures leave you not-deleted.

>+  // TODO: smartest way to pass nsURI == (spec, charset)? 

we should work this out with bz and others at the workweek: I think that for all the URIs we care about here (which are limited to HTTP), we should just pass URIs as normalized UTF8, but we could also choose to create a parsed structure for them, since we'll have to parse them on the child side anyway, and serialize the parsed version across the wire.

>+HttpChannelChild::GetResponseStatus(PRUint32 *aResponseStatus)

We'll need this pretty quick, I suspect.

>diff --git a/netwerk/protocol/http/src/HttpChannelParent.cpp b/netwerk/protocol/http/src/HttpChannelParent.cpp

>+NS_IMETHODIMP
>+HttpChannelParent::OnDataAvailable(nsIRequest *aRequest, 
>+                                   nsISupports *aContext, 
>+                                   nsIInputStream *aInputStream, 
>+                                   PRUint32 aOffset, 
>+                                   PRUint32 aCount)
>+{
>+  LOG(("HttpChannelParent::OnDataAvailable [this=%x]\n", this));
>+ 
>+  nsresult rv;
>+
>+  char *buffer = (char*)malloc(sizeof(char) * aCount);
>+  if (!buffer)
>+    return NS_ERROR_OUT_OF_MEMORY;
>+
>+  // TODO: Is there a better way to read data into a string?
>+  PRUint32 bytesRead;
>+  rv = aInputStream->Read(buffer, aCount, &bytesRead);

Yes, especially because I'm pretty sure you know the channel type involved: you could, instead of using a copying ->Read call, just create a nsDependentCSubstring(start, end) and, I think, completely avoid an allocation.

Of course, ugh, the parameter is declared a const nsCString&, and I'm not sure that nsDependentCSubstring-is-a-nsCString. That's unfortunate in this case.

>+  if (!NS_SUCCEEDED(rv)) {
>+    free(buffer);
>+    return rv;              // TODO: figure out error handling
>+  }
>+  nsCString data;
>+  data.Adopt(buffer, bytesRead);

Anyway, .Adopt buffers allocated with NS_Alloc, not malloc.

>+
>+  if (!SendOnDataAvailable(data, aOffset, bytesRead)) {
>+    // TODO Fatal error--child dead?  
>+    // Need to Cancel/cleanup?
>+    // What error code to return here?
>+    return NS_ERROR_UNEXPECTED; 

The cancel/cleanup can happen in the ActorDestroy callback, so I don't think you need to do anything other than return NS_ERROR_UNEXPECTED here... although perhaps we ought to invent a new nsError.h code for IPC errors.


> // Header file contents
> class HttpChannelParent :
>-  public PHttpChannelParent
>+    public PHttpChannelParent
>+  , public nsIStreamListener

This is weird indentation, I suggest

class HttpChannelParent
  : public PHttpChannelParent
  , public nsIStreamListener

clearing review, since I know you have further refcounting/dealloc changes pending

Comment 2

[Jason Duell]

Attachment: basic HTTP functionality for e10s

OK, here's an updated patch for e10s HTTP.  For now I'm assigning review to bz/biesi.  It would be lovely to have this in the tree by the end of the year :)

> I don't think prefs are the right vehicle for this. Until it "mostly works" I
> think we should just use an environment-variable check.

Replaced with check of NECKO_E10S_HTTP environment variable.

> static inline ...

Changed to "inline". Note: We have other instances of static inline in some of the existing http code.  Should I remove them?  

> >+void NeckoChild::DestroyNeckoChild()
>
> I don't think we ever need to explicitly destroy this actor: it will go away
> all by itself when the toplevel PContentProcess shuts down.

Cjones tells me that that would currently work, but may change, so I'm leaving in the explicit destruction code for now.  We can always remove it later, and it does no harm.

> I wonder if we shouldn't skip the PNecko protocol altogether and just put
> whatever toplevel messages you need directly on PContentProcess.

Quite possibly.  Let's wait on it for now, and perhaps do it when things mature.  During active development it has the advantage that there's less chance of merge conflicts, and it also means I don't have to re-link liblayout every time I change the file.

> NS_INTERFACE_MAP_ENTRY(nsITransportEventSink)
> NS_INTERFACE_MAP_ENTRY(...)
>
> I'm pretty sure this is an implementation detail and can/should be removed.

If we're positive that we won't need an interface, then yes, I'll get rid of it.  Otherwise, I figure it's easier to keep the DROP_DEAD impls of these things in until I know for sure that we don't need them.  A lot of our code does things like quietly return if a cast to an XPCOM interface fails;  I'd rather have them drop dead loudly.

That said, I'm also pretty sure that we can get rid of some of these, since we shouldn't need them on the client side.  Which are we 100% sure of?

>+  nsresult rv = mChildListener->OnStartRequest(this, mChildListenerContext);
>+  if (!NS_SUCCEEDED(rv)) {
>+    return false;  // kills child?  TODO:  what to do here?  Cancel request?
>
> What is the in-process behavior if OnStartRequest fails? returning false is
> definitely the wrong solution.

Yes, this needs to cancel the request instead, and return true.  I've noted this in the comments--implementing Cancel is future work.

> Get/SetOriginalURI, GetUri, Get/SetNotificationCallbacks, GetResponseStatus

Implemented.  The parent class also implements a stub callback, which will eventually get fleshed out to either send msgs to the callbacks on the child, or hook up the appropriate chrome thingy (for auth alerts, etc.).

> GetContentType, GetContentLength

Shoehorned these in for now: eventually this code will change when we copy the full nsIResponseHead.  I haven't implemented the Set versions.

> >+  // TODO: Is there a better way to read data into a string?

Punting for now: added comment about how we can optimize later with shmem.

Other Notes: 

I'm still passing URIs as (Spec, charSet).  I'll open a bug for the issue of how we might better serialize them.

nsHttpChannel tracks the lifecycle of the channel using a grab-bag of variables: mIsPending, mWasOpened, whether mResponseHead != null.  There are some states where calls are probably not sensible (ex: calling SetOriginalURI after AsyncOpen), and would cause us extra work in e10s if chrome needs to be updated, so I've explicitly made them illegal for now (we'll see if they wind up being needed).  To do this I've introduced an 'mState' enum variable.  This will eventually be replaced by using the IPDL protocol state once we implement IPDL state transition checking.

RefCounting is still broken, and this code leaks memory.  That's another TODO for future work.  As discussed a bit on IRC, refcounting is a bit tricky here, since we want to trigger IPDL destruction when we're done with the channel, i.e.  when refcnt==0, so we don't actually want to always delete the class then.  I'm planning to override Release, and if refcnt==0 but IPDL destruction hasn't happened yet, init IPDL shutdown and then actually delete in NeckoChild::DeallocPHttpChannel().

Comment 3

[Jason Duell]

Comment on attachment 418007 [details] [diff] [review]
basic HTTP functionality for e10s 

biesi has told me he's too busy for this review, so moving to bsmedberg/bz for r/sr.   bz tells me he should be done with his review by 1/15, so bsmedberg may want to hold off for a final version with and changes bz suggests.

I will be meeting with biesi on 1/19 at google to fill him in on the basic design here, so he can stay abreast of what we're up to with e10s necko.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 418007 [details] [diff] [review]
basic HTTP functionality for e10s 

>+++ b/netwerk/ipc/NeckoChild.cpp
> PHttpChannelChild* 
> NeckoChild::AllocPHttpChannel()
> {
>+  // We don't allocate here: see HttpChannelChild::AsyncOpen()
>+  NS_RUNTIMEABORT("AllocPHttpChannel should not be called");

I think some documentation on the creation/destruction/ownership model here is in order...  Who's responsible for creating PHttpChannelChild objects?  Where is the addref that matches the release in DeallocPHttpChannel?

>+++ b/netwerk/protocol/http/src/HttpChannelChild.cpp
>+HttpChannelChild::Init(nsIURI *uri)
>+/*  TODO: split this out into a separate function so we can share with
>+ *  nsHttpChannel.  

Yes.  Please file a followup bug and make this a FIXME comment.

>+   // TODO: avoid copying data if possible (need version of stringstream that
>+   // can temporarily wrap string?)

The problem is that streams are refcounted... and strings may or may not be.

That said, if the stringstream could use+refcount a string buffer, that would work.  Shouldn't be too hard to do; file a bug?

>+HttpChannelChild::RecvOnStopRequest(const nsresult& statusCode)

Need to drop mChildListener and mChildListenerContext after the OnStopRequest call, right?

>+HttpChannelChild::Open(nsIInputStream **retval)
>+{
>+  return NS_ERROR_NOT_IMPLEMENTED;

Why not DROP_DEAD()?

>+HttpChannelChild::AsyncOpen(nsIStreamListener *listener, nsISupports *aContext)
>+   * Only synchronous error check in AsyncOpen is for port safety.

Longer-term, that will need to happen in chrome, either in addition to or instead of the check here.

>+  // TODO: smartest way to pass nsURI == (spec, charset)? 

Seems like it, yeah, for now...  Can we structify that so we can change how we pass it without changing various protocol code?

>+  if (!SendAsyncOpen(mSpec, charset, originalSpec, originalCharset, 
>+                     docSpec, docCharset, mLoadFlags)) {
>+    // IPDL error: our destructor will be called automatically
>+    // -- TODO: verify that that's the case :)
>+    return NS_ERROR_FAILURE;

Worth dropping the listener/context here?

>+HttpChannelChild::SetInheritApplicationCache(PRBool aInheritApplicationCache)
>+{
>+  // TODO:  Browser calls this early: do something?

File a followup to track. Same for SetChooseApplicationCache.

>+++ b/netwerk/protocol/http/src/HttpChannelParent.cpp
>+HttpChannelParent::OnDataAvailable(nsIRequest *aRequest, 
>+  // TODO: Is there a better way to read data into a string?

BeginWriting on the string and pass that pointer to the stream to read into?

Can be a followup.

sr=bzbarsky with the above nits.

Comment 5

[Jason Duell]

Attachment: revised with bz's comments

Addressed bz's comments: now need +r from bsmedberg.

> I think some documentation on the creation/destruction/ownership model
> here is in order...  Who's responsible for creating PHttpChannelChild
> objects?  Where is the addref that matches the release in
> DeallocPHttpChannel?

I'll document this when I fix bug 536316 (refcounting is broken).

>+++ b/netwerk/protocol/http/src/HttpChannelChild.cpp
>+HttpChannelChild::Init(nsIURI *uri)
>+/*  TODO: split this out into a separate function so we can share with
>+ *  nsHttpChannel.

Filed bug 541017.

> That said, if the stringstream could use+refcount a string buffer,
> that would work.  Shouldn't be too hard to do; file a bug?

I believe I've fixed this by passing NS_ASSIGNMENT_DEPEND instead of NS_ASSIGNMENT_COPY.  While this doesn't actually addref the string buffer, we should be OK, in that the contract for OnDataAvail requires that the client read all the data in the stream during OnDataAvail (so we're guaranteed the string is still in scope).  Biesi tells me that the current impl is actually more tolerant, and will actually allow you to only read part of the data, then get the rest in a later OnDataAvail call.  This won't support that--I added a comment to that effect.  If we discover lots of clients depend on the more tolerant behavior, we can revisit.

>>+HttpChannelChild::RecvOnStopRequest(const nsresult& statusCode)
>
>Need to drop mChildListener and mChildListenerContext after the OnStopRequest
>call, right?

Fixed.

>>+HttpChannelChild::Open(nsIInputStream **retval)
>>+{
>>+  return NS_ERROR_NOT_IMPLEMENTED;
>
>Why not DROP_DEAD()?

I just copied and pasted from nsHttpChannel, so I assume keeping the same behavior is OK.

>>+HttpChannelChild::AsyncOpen(nsIStreamListener *listener, nsISupports *aContext)
>>+   * Only synchronous error check in AsyncOpen is for port safety.
>
>Longer-term, that will need to happen in chrome, either in addition to or
>instead of the check here.

The check is also duplicated on the parent--the check is done in the child as well just so we can return an error immediately.  There may be some issues keeping the port lists in sync btw parent/child--filed bug
541025.   But not likely to be a problem for now.

>>+  // TODO: smartest way to pass nsURI == (spec, charset)?
>
>Seems like it, yeah, for now...  Can we structify that so we can change how we
>pass it without changing various protocol code?

Filed bug 541174.

>>+  if (!SendAsyncOpen(mSpec, charset, originalSpec, originalCharset,
>>+    return NS_ERROR_FAILURE;
>
>Worth dropping the listener/context here?

Yes. Fixed.

>>+HttpChannelChild::SetInheritApplicationCache(PRBool aInheritApplicationCache)
>>+{
>>+  // TODO:  Browser calls this early: do something?
>
>File a followup to track. Same for SetChooseApplicationCache.

I'm assuming this can just be part of bug 536295 (get app cache working under e10s).  Stuck bug # in the comment.

>>+++ b/netwerk/protocol/http/src/HttpChannelParent.cpp
>>+HttpChannelParent::OnDataAvailable(nsIRequest *aRequest,
>>+  // TODO: Is there a better way to read data into a string?
>
>BeginWriting on the string and pass that pointer to the stream to read into?

OK, I changed to use Begin/EndWriting.

>sr=bzbarsky with the above nits.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> This won't support that--I added a comment to that effect. 

"won't support" in the "crashes if you try" sense, right?

Would it make sense to close the stream after the ODA call returns?

> I just copied and pasted from nsHttpChannel,

nsHttpChannel implements Open.  So I'm not sure what you copied...

Comment 7

[Jason Duell]

Attachment: updated with 2 more fixes from bz

> Would it make sense to close the stream after the ODA call returns?

Yes indeed!  Thanks.

> nsHttpChannel implements Open. 

Hmm.  I've changed to DROP_DEAD() for now.  But I assume (?) we should be able to support Open() the same way as nsHttpChannel (call NS_ImplementChannelOpen). Is it worth opening a bug for this?  I've heard that providing sync Open was a bad design, but it's part of the interface, so we should have a plan.

Comment 8

[Boris Zbarsky [:bzbarsky]]

If we're not planning to drop open() altogether, we need to keep supporting it.  It's certainly used.  NS_ImplementChannelOpen is a fine way to do that.

Comment 9

[Jason Duell]

Attachment: Implemented Open()

OK, I've updated the patch with an Open impl based on NS_ImplementChannelOpen.

Comment 10

[Jason Duell]

Attachment: Actually implement Open()

Whoops--attached wrong version.   This is the one.

Comment 11

[Benjamin Smedberg]

Attachment: Review notes for attachment 423304

Comment 12

[Benjamin Smedberg]

Comment on attachment 423304 [details] [diff] [review]
Actually implement Open()

r=me with review notes in attachment 425525 [details]

Comment 13

[Jason Duell]

Attachment: Updated with bsmedbergs fixes and style suggestions.

Not many code changes here, but lots of textual ones (removing/reformatting comments).

Comment 14

[Jason Duell]

Attachment: Fixes subtle issues with chromium interfering with NSPR logging.

This version contains the http module's "fix" for the missing log issue mentioned in bug 545995.  Fixes broken tinderbox builds.

Comment 15

[Jason Duell]

Attachment: Patch that also fixes bug 545995

oops--attached the wrong patch previously.  Sigh...

Comment 16

[Jason Duell]

http://hg.mozilla.org/projects/electrolysis/rev/959add9a851a