567267 - e10s: nsIEncodedChannel

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Jason Duell]

We need to support nsIEncodedChannel in the child.  GetContentEncodings seems to be pretty simple--just move to base class, all the logic can be done in the child.

Set/GetApplyConversion looks trickier.  For cases where SetApplyConversion is called before AsyncOpen, it's easy--just set the same value on the chrome nsHttpChannel.  But we've got a couple of uses of SetApplyConversion in OnStartRequest:

 http://mxr.mozilla.org/mozilla-central/source/uriloader/exthandler/nsExternalHelperAppService.cpp#1535

 http://mxr.mozilla.org/mozilla-central/source/embedding/components/webbrowserpersist/src/nsWebBrowserPersist.cpp#693
 
The existing nsHttpChannel logic assumes that OnStartRequest is called synchronously, i.e. that if the listener has called SetApplyConversion it is already available by the time ApplyContentConversions() is called a few lines later:

  http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/src/nsHttpChannel.cpp#828
   
I'm not sure how to proceed with this.  Ideally, we can move the logic that decides to call SetApplyConversion to chrome.  But for nsExternalHelper, at least, there seems to be some goop involving storing data with the channel as the hash key--haven't looked at it enough to know how hard it would be to move to chrome.  

Bz/biesi, thoughts?

Comment 1

[Boris Zbarsky [:bzbarsky]]

> But for nsExternalHelper, at least, there seems to be some goop involving
> storing data with the channel as the hash key--haven't looked at it enough to
> know how hard it would be to move to chrome. 

The external helper app service needs to live in chrome.  The problem is that our current short-term plan of shipping the data to content and then back to chrome when doing the external helper app stuff means we still can't do a sync OnStartRequest here, right?  Do we have a gzip encoder?  We may have to bite the bullet and just re-gzip in some cases (though that sounds really annoying).

The other option is to not start sending any data until the OnStartRequest in the child returns, of course.  Not sure how hard that is to do.

Comment 2

[Christian :Biesinger (don't email me, ping me on IRC)]

Well, you can Suspend() the channel, right?

Comment 3

[Christian :Biesinger (don't email me, ping me on IRC)]

So what might work is: HttpChannelParent Suspend()s the channel in OnStartRequest; the child tells the parent when it's done with OnStartRequest and whether it wants conversion;  nsHttpChannel moves ApplyContentConversion from CallOnStartRequest to the first OnDataAvailable call.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Yeah, that should work.

Comment 5

[Brian Crowder]

Attachment: move nsIEncodedChannel support to HttpBaseChannel (wip)

Another spin of this coming in a bit

Comment 6

[Brian Crowder]

Attachment: move nsIEncodedChannel support to HttpBaseChannel (wip v2)

This adds a check to make sure we are not using SetApplyConversion before OnStartRequest has happened

Comment 7

[Jason Duell]

Comment on attachment 446823 [details] [diff] [review]
move nsIEncodedChannel support to HttpBaseChannel (wip v2)

Looks good so far. Let's change mRequestStarted to "mOnStartReqCalled".  I find that more intuitive.

Next (do this as a separate patch, to make my review easier):  Following comment #3, have nsHttpChannel::CallOnStartRequest() call Suspend after the listener's OnStartRequest is called.  Then figure out how to move ApplyContentConversions to some point before the listener's OnDataAvailable is called.  Do this only if the nsHttpChannel is servicing a content process request, i.e. if mRemoteChannel is set.  Otherwise keep the same logic we've got now (there's no point suspending the channel for a channel that originates in chrome).

You'll need to add a "StartRequestCompleted(bool applyConversion)" method to PHttpChannel.ipdl.   It will go from child to parent, and will be async.  Call this method in RecvOnStartRequest, after the listener's OnStartRequest has been called.   RecvStartRequestCompleted() should simply set SetApplyConversion on the nsHttpChannel, and then call Resume() on it.

One wrinkle:  it's possible that the client code on the child could call Suspend and/or Resume during it's OnStartRequest.  We need to make sure our hacky Suspend/Resume doesn't interfere with that.  I.e. if we call suspend in CallOnStartRequest, then the listener in the child calls Suspend in it's OnStartRequest, it would be an error for us to Resume the channel in RecvStartRequestCompleted().   We don't actually have Suspend/Resume working yet from the child, so you can skip this part for now and get everything working.  But you may want to think about it, 'cause we're going to need it soon.  My guess is that we could do something like hack suspend/resume on the child to keep an "mIsSuspended" variable updated, and when we call SendStartRequestCompleted we could pass another bool indicating if the channel is currently suspended, and if so, don't resume it.

Comment 8

[Jason Duell]

I suppose one obvious place to put the ApplyContentConversions call is in RecvStartRequestCompleted, before Resume.

Comment 9

[Brian Crowder]

Attachment: move nsIEncodedChannel support to HttpBaseChannel (wip v3)

Updated by jduell, this one *actually* builds.

Comment 10

[Brian Crowder]

Attachment: implement suspend/resume (wip)

I'm sure this is missing stuff, but would still love to get feedback from biesi, bz, and jduell

Comment 11

[Brian Crowder]

Comment on attachment 449366 [details] [diff] [review]
implement suspend/resume (wip)

Some remarks:  I think we're okay on Suspend/Resume nesting, as these routines keep counters.  I'm worried about short-circuiting in my new Suspend() code before the existing offline-cache handling code.  Should I just move that stuff earlier?

Comment 12

[Jason Duell]

Comment on attachment 449366 [details] [diff] [review]
implement suspend/resume (wip)

bz/biesi:  Please correct me if I'm wrong about the cache listeners depending on the gzip encoding listener already being installed to work properly.


>diff --git a/netwerk/protocol/http/src/HttpChannelChild.cpp b/netwerk/protocol/http/src/HttpChannelChild.cpp
>--- a/netwerk/protocol/http/src/HttpChannelChild.cpp
>+++ b/netwerk/protocol/http/src/HttpChannelChild.cpp
>@@ -111,6 +111,7 @@ HttpChannelChild::RecvOnStartRequest(con
> 
>   nsresult rv = mListener->OnStartRequest(this, mListenerContext);
>   mRequestStarted = PR_TRUE;
>+  SendStartRequestCompleted(mApplyConversion);
>   if (!NS_SUCCEEDED(rv)) {
>     // TODO: Cancel request:
>     //  - Send Cancel msg to parent 

Also send "rv" as a parameter to SendStartReqCompleted, and if it is !NS_SUCCEEDED, cancel the nsHttpChannel with it.  Remove the TODO Cancel comment.


>diff --git a/netwerk/protocol/http/src/HttpChannelParent.cpp b/netwerk/protocol/http/src/HttpChannelParent.cpp
>--- a/netwerk/protocol/http/src/HttpChannelParent.cpp
>+++ b/netwerk/protocol/http/src/HttpChannelParent.cpp
>@@ -168,6 +168,23 @@ HttpChannelParent::RecvAsyncOpen(const I
>   return true;
> }
> 
>+bool
>+HttpChannelParent::RecvStartRequestCompleted(const bool& applyConversion)
>+{
>+  LOG(("HttpChannelParent::RecvStartRequestCompleted [this=%x]\n", this));
>+  nsHttpChannel *httpChan = static_cast<nsHttpChannel *>(mChannel.get());
>+
>+  httpChan->SetApplyConversion(applyConversion);
>+  
>+  nsresult rv = httpChan->ApplyContentConversions();
>+  if (NS_FAILED(rv)) return false;
>+
>+  rv = httpChan->Resume();
>+  if (NS_FAILED(rv)) return false;
>+
>+  return true;
>+}

So it looks like ApplyContentConversions always returns NS_OK, as does the SetApplyConversions method.  And most uses of Resume that I see in the Mozilla codebase don't check for errors, so you may not actually need any error checking here, as near as I can tell.  If you do keep any of it, you don't want to return false (returning false from an IPDL method basically means, "kill the child process").  Instead you'd call Cancel() on the nsHttpChannel.

As described below, this method is going to wind up calling more stuff than this.

As we've discussed on IRC, it turns out the nsHttpChannel (and the input streams below it) already handle nested suspend/resume, so we don't need to implement anything special to handle the case if the stream is suspended by the necko client in OnStartRequest.

>diff --git a/netwerk/protocol/http/src/nsHttpChannel.cpp b/netwerk/protocol/http/src/nsHttpChannel.cpp
>--- a/netwerk/protocol/http/src/nsHttpChannel.cpp
>+++ b/netwerk/protocol/http/src/nsHttpChannel.cpp
>@@ -710,6 +710,14 @@ nsHttpChannel::CallOnStartRequest()
>     nsresult rv = mListener->OnStartRequest(this, mListenerContext);
>     if (NS_FAILED(rv)) return rv;
> 
>+    // Suspend remote channels, we'll finish the rest of this work later
>+    if (mRemoteChannel) {
>+      rv = Suspend();
>+      if (NS_FAILED(rv)) return rv;
>+
>+      return NS_OK; // we'll finish up the rest of this later for remote channels
>+    }
>+
>     // install stream converter if required
>     rv = ApplyContentConversions();
>     if (NS_FAILED(rv)) return rv;

So if mRemoteChannel, you're simply skipping the offline cache logic that follows the original call to ApplyContentConversions().  That's not ok.  But that logic isn't safe to run until ApplyContentConversions() is complete: if the web site is returning gzipped HTML, for instance, ApplyCC winds up inserting a listener that unzips the content before OnDataAvailable is called, and InstallOfflineCacheListener is relying on getting that unzipped data, AFAICT.  So I think you're going to want to split out everything from the ApplyContentConversions to the end of the function into a new function ("FinishOnStartRequest") that will call ApplyContentConversion and also do the app cache logic.  Call it synchronously from CallOnStartRequest if !mRemoteChannel, and otherwise it will be called in RecvStartRequestCompleted.

Looking at the stack of callers up to this point, we will also need to delay the logic that installs the regular cache listener in nsHttpChannel::ProcessNormal(), right after CallOnStartRequest is called:
 
  http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/src/nsHttpChannel.cpp#1136

InstallCacheListener also appears to implicitly depend on the gzip (or other) converting listener already being installed.  So move that logic into "FinishOnStartRequest" as well.  Add a boolean "installCacheListener" flag to the arguments, since CallOnStartRequest() is called in many places in the code, and we only want the call from ProcessNormal() to install the listener.

RecvStartRequestCompleted will need to handle any non NS_OK code returned from FinishOnStartReq.  Call Cancel on the nsHttpChannel with the return code if !NS_SUCCEEDED.

Comment 13

[Brian Crowder]

Attachment: implement suspend/resume (wip v2)

This seems to work as expected.  Jason, would you please give this the once(or twice)-over and then tag someone else for review?

Comment 14

[Boris Zbarsky [:bzbarsky]]

> bz/biesi:  Please correct me if I'm wrong about the cache listeners 

You're correct.  The offline cache expects to cache the actual data, after content encodings have been decoded.

Comment 15

[Brian Crowder]

Attachment: updated the 1st stage patch to e10s tip

Comment 16

[Brian Crowder]

Attachment: updated the 2nd stage patch to e10s tip

Comment 17

[Brian Crowder]

Attachment: updated the 1st stage patch to e10s tip

Argh, wrong attachment

Comment 18

[Brian Crowder]

Attachment: updated the 2nd stage patch to e10s tip

Again, wrong attachment

Comment 19

[Brian Crowder]

The last two patches have some cruft which I'll clean up later tonight.

Comment 20

[Josh Matthews [:jdm]]

>+  // We send rv back to the parent to allow for cancellation on failures
>+  SendStartRequestCompleted(mApplyConversion, rv);
>+  if (!NS_SUCCEEDED(rv))
>+    return false;

Either NS_FAILED(rv) or just a straight |return NS_SUCCEEDED(rv)|.

However, returning false is going to kill the child process in the near future.  Is that actually what we want here?  I suspect not.  The same comment applies to all other places we're currently returning false in this patch.

>+    if (mRemoteChannel) {
>+      rv = Suspend();
>+      if (NS_FAILED(rv)) return rv;
>+
>+      return NS_OK; // we'll finish up the rest of this later for remote channels
>+    }

Could we just |return rv| here unconditionally?

Comment 21

[Josh Matthews [:jdm]]

Nits from the 1st stage:

>+HttpBaseChannel::nsContentEncodings::nsContentEncodings(nsIHttpChannel* aChannel,
>+                                                        const char* aEncodingHeader) :
>+    mEncodingHeader(aEncodingHeader), mChannel(aChannel), mReady(PR_FALSE)

: should go on the next line.

>+      aNextEncoding.AssignLiteral(APPLICATION_COMPRESS);
>+                                       
>+      haveType = PR_TRUE;

Unneeded extra newline.

>+  if (! haveType) {

Extraneous space after !.

Comment 22

[Honza Bambas (:mayhemer)]

Is still any work done here?  Can be Josh's comments be taken as a review?

Few toughs:
- instead of suspending the whole channel it would be better to suspend just the transaction
- this has performance impact: what about to bypass this whole mechanism when the mime type doesn't pass IsAcceptableEncoding?  maybe a significant number of real-life cases
- in HttpChannelParent::RecvStartRequestCompleted you always have to call Resume to restart the transaction otherwise you won't get OnStopRequest and the channel will hang; calling just Cancel doesn't ensure that
- would be great to have tests for this (xpcshell)

Comment 23

[Brian Crowder]

Attachment: rev for reviews

This fixes comments from :jdm's review.

Comment 24

[Brian Crowder]

Attachment: rev for reviews (pt2)

This fixes jdm's comments, and addresses Honza's concerns about Cancel/Resume behavior (I hope).  Regarding other concerns:

1) I'd like to handle performance concerns in a follow-up bug, if possible.
2) I'm not sure what sort of tests we'd implement for this functionality, can you offer any suggestions?

Comment 25

[Honza Bambas (:mayhemer)]

(In reply to comment #24)
> Created attachment 462493 [details] [diff] [review]
> rev for reviews (pt2)
> 
> This fixes jdm's comments, and addresses Honza's concerns about Cancel/Resume
> behavior (I hope).  Regarding other concerns:
> 

I don't think you want to call FinishOnStartRequest when rv is a failure.

As I think of it, it is ok to suspend the whole channel, we will probably need this also for responses satisfied from cache, although those will never demand content encoding. 

> 1) I'd like to handle performance concerns in a follow-up bug, if possible.
> 2) I'm not sure what sort of tests we'd implement for this functionality, can
> you offer any suggestions?

Probably create an xpc-shell test.  Example of how to work with encoded content in such a test is netwerk/test/unit/test_gzipped_206.js.  Craete e.g test_bug567267.js and check you get decoded content on OnDataAvailable + no errors.  To make it work under e10s, you will want to wrap it in netwerk/test/unit_ipc/test_bug567267_wrap.js.  See that directory for existing examples.

Comment 26

[Brian Crowder]

jduell:  Do you agree with Honza about not calling FinishOnStartRequest here?

Comment 27

[Brian Crowder]

Adding jduell to answer comment #26

Comment 28

[Honza Bambas (:mayhemer)]

He reported the bug, he gets comments automatically.

As I see the current implementation, it is not necessary to bypass FinishOnStartRequest.  mCanceled is true and mApplyConversion is false, so FinishOnStartRequest actually does nothing.  But the implementation may change.

Comment 29

[Jason Duell]

Comment on attachment 462490 [details] [diff] [review]
rev for reviews

rename mRequestStarted -> "mOnStartRequestCalled" as per earlier comment.

Also, mRequestStarted/mOnStartRequestCalled is currently only getting set in HttpChannelChild, which means that it's always false in nsHttpChannel.  This has no negative conquences, but it pretty unintuitive.  Find a place to set it (it needs to be after you call SetApplyConversion in RecvStartRequestCompleted, so FinishOnStartRequest seems the logical place).


+  if (CaseInsensitiveFindInReadable(NS_LITERAL_CSTRING("gzip"), 
+                                    start,
+                                    end)) {
+    aNextEncoding.AssignLiteral(APPLICATION_GZIP);
+    haveType = PR_TRUE;
+  }

Let's get rid of this crappy, one-arg per line formatting, i.e.

+  if (CaseInsensitiveFindInReadable(NS_LITERAL_CSTRING("gzip"), start, end);

Patch #1 is not applying cleanly to e10s tree, at least.  Fine if it does apply to m-c;   otherwise, please rebase to e10s (I think we'll want to land there first).

Comment 30

[Jason Duell]

Comment on attachment 462493 [details] [diff] [review]
rev for reviews (pt2)

You haven't done the last part of comment 12--moving the logic in ProcessNormal into FinishOnStartRequest.  (The logic to be moved is the installation of the cache listener--note that it's been moved to ContinueProcessNormal.  It still needs to be moved into FinishOnStartRequest, with the boolean).

> As I see the current implementation, it is not necessary to bypass
> FinishOnStartRequest.  mCanceled is true and mApplyConversion is false, so
> FinishOnStartRequest actually does nothing.

Actually mApplyConversion could be true, and we'd then set in on the nsHttpChannel.  Which I don't think does any harm.  So we might as well keep calling FinishOnStartRequest, even if cancelled.  Especially since we're going to be setting mOnStartRequestCalled in it.

Honza:  re: suspending only the transaction, and/or bypassing this whole mechanism when possible.  This code is going to eventually change hopefully to move the decision to apply content encoding into chrome, so we don't need all this IPDL traffic.  But if there are known cases where we don't need content-encoding (sounds like you're saying we don't when a request is supplied by the cache?), then we could do those optimizations now.   What do you think?

Comment 31

[Brian Crowder]

Attachment: move encoding to base, review rev 2

Comment 32

[Brian Crowder]

Attachment: suspend/resume (review rev 2)

Apologies in advance if I've again missed something, thanks for the thorough reviews!

Comment 33

[Honza Bambas (:mayhemer)]

We have to remember the installCacheListener flag to correctly pass it to FinishOnStartRequest in RecvStartRequestCompleted.

BTW: I still think that when OnStartRequest failed on the child, we shouldn't call FinishOnStartRequest.  We don't call code after OnStartRequest when it fails now, so why to start doing it?

Comment 34

[Honza Bambas (:mayhemer)]

And please rename FinishOnStartRequest to FinishCallOnStartRequest, it's absolutely confusing.

Comment 35

[Brendan Eich [:brendan]]

Sometimes I use a "tail" suffix for such code factoring, so here it would be CallOnStartRequestTail. The Finish verb is meta in the current patch's usage: it applies to the static source, it does not mean a "finish" at runtime.

/be

Comment 36

[Brian Crowder]

Attachment: suspend/resume (review rev 3)

With Honza's last remarks about carrying the caching flag forward, a test-case added, and Brendan's naming-suggestion.  More feedback welcome.

Comment 37

[Boris Zbarsky [:bzbarsky]]

So unless I'm misreading the patch it looks like it will break application of nsUnknownDecoder to HTTP channels (because we won't pump any data into the unknown decoder, etc).

Then again, that setup was already broken with SetApplyConversion stuff (which admittedly is a rare case).

Can we just switch nsUnknownDecoder to use NS_CONTENT_SNIFFER_CATEGORY instead of sitting between the channel and its listener?  Check with biesi?

Comment 38

[Brian Crowder]

Until someone convinces me otherwise, I'm closing this bug out as INVALID.  The Parent->Child->Parent forwarding seems to solve this issue for ExternalHelperAppService.

Comment 39

[Boris Zbarsky [:bzbarsky]]

In that we don't start sending data until after we've done the roundtrip with OnStartRequest or something?  If not, then how could it possibly work?

Comment 40

[Boris Zbarsky [:bzbarsky]]

Reopening per IRC discussion; there really is work to be done here.  And ideally we'd have tests for this; manual ones worst-case.

Comment 41

[Boris Zbarsky [:bzbarsky]]

irc log, lightly edited:

Data flow is:

HTTPChannel (in chrome) -> HTTPChannelParent -> HTTPChannelChild -> child uriloader -> child ExtHandler -> parent ExtHandler -> parent DoContent,
> ok
> great
> so you're proposing that there be code in the "parent ExtHandler" step that
  does decompression
> right?
<crowder> right
> ok
> Like I said, that should work
<crowder> And that the child exthandler would disable decoding on the channel
> right
> so none of the earlier steps in the flow do any decompression in this
  situation
> and in fact, what needs to happen is that HTTPChannelParent disables
  decompression on HTTPChannel
<crowder> bz: That's my assertion, I'm not 100% on that
> HTTPChannelChild will then need to either decompress or not depending on what
  its stream listener tells it
> and child exthandler will tell it to not do it

Comment 42

[Christian :Biesinger (don't email me, ping me on IRC)]

two notes on that:
- HttpChannelParent will have to call SetApplyConversion(PR_FALSE) on the httpchannel
- HttpChannel/nsIEncodedChannel will need an API to return the streamconverter mime type corresponding to the content-encoding of the channel (or I guess a list of those, though we don't currently support nested encodings)

Comment 43

[Christian :Biesinger (don't email me, ping me on IRC)]

Maybe I should expand a bit more on that:

The HttpChannelParent has to call SetApplyConversion(FALSE) because waiting for the child to call that will be too late - this has to be called during OnStartRequest, because HttpChannel will check it right after calling OnStartRequest. See http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannel.cpp#787. Therefore HttpChannelChild will have to also take care of inserting the stream converter for the content-encoding if its listener wants conversion, which will be all of them except for downloads.

For downloads the child won't want to do any decoding, instead the parent exthandler (as mentioned) will want to insert that stream converter in the listener chain, except for the case where it currently calls SetApplyConversion(PR_FALSE).

Comment 44

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #42)
> - HttpChannel/nsIEncodedChannel will need an API to return the streamconverter
> mime type corresponding to the content-encoding of the channel (or I guess a
> list of those, though we don't currently support nested encodings)

As I just found out, we already have that API in form of nsIEncodedChannel::contentEncodings.

Comment 45

[Brian Crowder]

Attachment: the beginnings of a testcase

Comment 46

[Brian Crowder]

Attachment: rebased this and removed mOnStartRequestCalled

mOnStartRequestCalled is no longer necessary because we don't need suspend/resume (not sure how it ended up in this half of the patch-set anyway).

Comment 47

[Brian Crowder]

Attachment: completed tests

First test is the "easy" case, we should pass through the data regardless (file extension is ".gz" and Content-Type is "application/x-gzip").

Second test changes Content-Type to "application/x-gzip", but we must still pass-through the data unchanged because of the ".gz" extension.

Final test does changes the extension to ".txt" and expects decoded data to arrive in the downloaded file.

Comment 48

[Brian Crowder]

Attachment: fix parent->child->parent encoding paths

This moves ApplyContentConversion() from nsHttpChannel to HttpBaseChannel and then uses it in HttpChannelChild::OnStartRequest.  Additionally it re-orders nsExternalAppHandler::OnStart request to cause the encodedChannel calculations to happen on the child side before bailing out.

This means that data needing to be decoded will be decoded here, on the child side, before being forwarded back to the parent to external handling.

jduell:  I'd like to have your feedback on the minor re-organization happening in HttpChannelChild::OnStartRequest.

Comment 49

[Brian Crowder]

Attachment: move management of this protocol out of PBrowser

I moved the management of the EAH protocol up from PBrowser to PContent to support the tests in this bug.  This means passing a null nsIInterfaceRequestor into uriloader::DoContent, but thanks to recent changes that's no longer a show-stopped; this all works in Fennec.  Eventually, I think, all of the aWindowContext/mWindow context stuff in nsExternalHelperAppService.cpp can go away.

Comment 50

[[e-mail]]

Will review this as my top priority tomorrow.

Comment 51

[[e-mail]]

Comment on attachment 474743 [details] [diff] [review]
rebased this and removed mOnStartRequestCalled

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

>+NS_IMETHODIMP
>+HttpBaseChannel::GetApplyConversion(PRBool *value)
>+{
>+  NS_ENSURE_ARG_POINTER(value);

No need.

>+NS_IMETHODIMP
>+HttpBaseChannel::GetContentEncodings(nsIUTF8StringEnumerator** aEncodings)
>+{
>+  NS_PRECONDITION(aEncodings, "Null out param");

No need.

>+  if (!mResponseHead) {
>+    *aEncodings = nsnull;
>+    return NS_OK;
>+  }
>+    
>+  const char *encoding = mResponseHead->PeekHeader(nsHttp::Content_Encoding);
>+  if (!encoding) {
>+    *aEncodings = nsnull;
>+    return NS_OK;
>+  }
>+  nsContentEncodings* enumerator = new nsContentEncodings(this, encoding);
>+  if (!enumerator)
>+    return NS_ERROR_OUT_OF_MEMORY;

No need.

>+HttpBaseChannel::nsContentEncodings::nsContentEncodings(nsIHttpChannel* aChannel,
>+                                                        const char* aEncodingHeader)
>+  : mEncodingHeader(aEncodingHeader), mChannel(aChannel), mReady(PR_FALSE)
>+{
>+  mCurEnd = aEncodingHeader + strlen(aEncodingHeader);
>+  mCurStart = mCurEnd;

Just move these up to the initializer line, and split them out one per line?

(I think that works, but it might not allow function calls in initializers, I forget.)

>+NS_IMETHODIMP
>+HttpBaseChannel::nsContentEncodings::HasMore(PRBool* aMoreEncodings)
>+{
>+  if (mReady) {
>+    *aMoreEncodings = PR_TRUE;
>+    return NS_OK;
>+  }
>+    

Extra whitespace on that line.

>+nsresult
>+HttpBaseChannel::nsContentEncodings::PrepareForNext(void)
>+{
>+  NS_PRECONDITION(mCurStart == mCurEnd, "Indeterminate state");

NS_ASSERTION please.

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

>@@ -3364,7 +3363,6 @@ NS_INTERFACE_MAP_BEGIN(nsHttpChannel)
>     NS_INTERFACE_MAP_ENTRY(nsIUploadChannel)
>     NS_INTERFACE_MAP_ENTRY(nsIUploadChannel2)
>     NS_INTERFACE_MAP_ENTRY(nsICacheListener)
>-    NS_INTERFACE_MAP_ENTRY(nsIEncodedChannel)

Uh. I think you still want this :)

>diff --git a/xpcom/glue/nsISupportsImpl.h b/xpcom/glue/nsISupportsImpl.h

>+#define NS_IMPL_ISUPPORTS_INHERITED8(Class, Super, i1, i2, i3, i4, i5, i6, i7, i8) \
>+    NS_IMPL_QUERY_INTERFACE_INHERITED8(Class, Super, i1, i2, i3, i4, i5, i6, i7, i8) \
>+    NS_IMPL_ADDREF_INHERITED(Class, Super)                                    \
>+    NS_IMPL_RELEASE_INHERITED(Class, Super)                                   \

Doesn't look like you use this?

r=dwitte with fixes.

Comment 52

[[e-mail]]

In future, when you're moving code, please split the patch into two parts: the first part is just cut-paste to the desired target (which won't compile); second is the relevant changes. This allows reviewers to just rubberstamp the first part, otherwise I have to read through everything to see if you made any modifications.

Comment 53

[[e-mail]]

Comment on attachment 475128 [details] [diff] [review]
completed tests

>+function initChildTestEnv()
>+{
>+  sendCommand('\
>+  const Cc = Components.classes;\
>+  const Ci = Components.interfaces;\
>+  const Cr = Components.results;\
>+  function WindowContext() { }\
>+\
>+  WindowContext.prototype = {\

Indent all your '\' at column 80 so they line up, and indent everything another level so it's indented past 'sendCommand'?

r=dwitte, with followup bug for adding a non-ipc test as well.

Comment 54

[[e-mail]]

Comment on attachment 475132 [details] [diff] [review]
fix parent->child->parent encoding paths

>diff --git a/netwerk/protocol/http/HttpBaseChannel.h b/netwerk/protocol/http/HttpBaseChannel.h

>@@ -140,6 +140,8 @@ public:
>   NS_IMETHOD GetApplyConversion(PRBool *value);
>   NS_IMETHOD SetApplyConversion(PRBool value);
>   NS_IMETHOD GetContentEncodings(nsIUTF8StringEnumerator** aEncodings);
>+  // Not really part of the nsIEncodedChannel interface, but topical here
>+  nsresult ApplyContentConversions();

Stick this in the private section?

r=dwitte.

Comment 55

[[e-mail]]

Comment on attachment 475181 [details] [diff] [review]
move management of this protocol out of PBrowser

r=dwitte.

Comment 56

[Brian Crowder]

Had to disable test_encoding.js for now, but I have a pretty good bead on the source of the issue and will post a patch shortly.

Comment 57

[Brian Crowder]

Attachment: reenable encoding tests

Easy fix: override the download-manager-ui to prevent the default/firefox implementation from trying to open a window.

Comment 58

[[e-mail]]

Comment on attachment 475728 [details] [diff] [review]
reenable encoding tests

Ok!

Comment 59

[Brian Crowder]

Marking this fixed.  I will resolve the Mac OS X xpcshell test failure in a follow-up.