1194052 - Append Principal to RedirectChain within LoadInfo before the channel is succesfully openend

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[Christoph Kerschbaumer [:ckerschb]]

Back when implementing Bug 1175803 we had several discussion when we should append the principal to the redirectChain. We finally ended up adding the principal after the channel succesfully openend.

After dealing with more use cases and the fact that security checks are happening within AsyncOpen(2), we decided we should append the redirect principal/uri to the loadInfo of the channel before asyncOpen() returned successfully. This way the securityManager can already inspect the redirectchain of the channel before making any decisions.

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1194052_redirectchain.patch

Pat, in the initial implementation [Bug 1175803] we decided to append to the redirectChain *after* a channel got successfully opened. Since the new ContentSecurityManager is consulted within ::AsyncOpen(2) it would be nice that the securityManager can already inspect the redirectchain on the loadInfo.

I am not sure whether these are the right places to put the code, nor do I know what we would like to do in the fallback case - should we remove the principal from the redirectchain?

Jonas, this is also a question for you, can we live with a polluted redirectchain in the fallback case, or no?

I am open for suggestions, but this patch seems to do what we want. Also passes the only test we have for the redirectchain: netwerk/test/unit/test_redirect_history.js

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

Additional note: Chatted with mrbkap on IRC. The reason this code works for e10s as well is because we call SetupReplacementChannel within HttpChannelChild [1] to keep the loadInfo in sync.

http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/HttpChannelChild.cpp#1111

Comment 3

[Honza Bambas (:mayhemer)]

I've already been suggesting this and asking about it during the initial implementation.  The answer was "no, we are OK to have it after AsyncOpen".  As I expected :DD it's not enough.

The way to do it right is to do a COPY of the LoadInfo, append the new principal, and arm the new channel with this copied LoadInfo.  If the redirect goes on, the info will be there prior AsyncOpen, if not, we have not polluted the old channel with unneeded info.

Comment 4

[Patrick McManus [:mcmanus]]

Comment on attachment 8647759 [details] [diff] [review]
bug_1194052_redirectchain.patch

Review of attachment 8647759 [details] [diff] [review]:
-----------------------------------------------------------------

honza's comment about the copy is a good one.. let's pursue that path and have honza review the next version

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I'm somewhat hesitant to copy the LoadInfo upon redirects. My hope was that we could eventually let addons store arbitrary information in a LoadInfo that they wanted to be associated with a network load and that should follow the load as it gets redirected and used in inner channels.

Another reason that I'm not excited to copy the loadinfo on redirects is that that means that loadinfo's would reuse loadinfos for inner channels, but not redirect, which leaves things a bit confusing.

But I agree that the current situation with the redirect chain is not good.

One potential solution is to move the redirect chain back to the channel object, rather than live on the loadinfo. That would be my ideal solution, but it looses the nice property that we have the redirect for *all* channels, and not just http channels. Technically this seems like the correct solution though, but one that doesn't seem realistic as long as addons can implement nsIChannel.

So my preferred solution here is that we move adding the redirect entry to the loadinfo before redirect, but remove it again if redirect failed. I agree it's not ideal, but it seems like the least bad solution.

But I guess I can live with copying the loadinfo too, though I suspect it'll cause headache in the future. But we can always reevaluate then.

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1194052_redirectchain.patch

(In reply to Patrick McManus [:mcmanus] from comment #4)
> honza's comment about the copy is a good one.. let's pursue that path and
> have honza review the next version

Copy of the loadInfo it is. Honza, you are not accepting review requests, so I am setting the needinfo flag with the hope you are going to do a review. Thanks for your input and feedback.

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #6)
> Created attachment 8648432 [details] [diff] [review]
> bug_1194052_redirectchain.patch
> 
> (In reply to Patrick McManus [:mcmanus] from comment #4)
> > honza's comment about the copy is a good one.. let's pursue that path and
> > have honza review the next version
> 
> Copy of the loadInfo it is. Honza, you are not accepting review requests, so
> I am setting the needinfo flag with the hope you are going to do a review.
> Thanks for your input and feedback.

Honza, how hard would it be to remove the entry from the redirect chain in case of a fallback? If it's not too complicated then maybe we should do that instead of copying the LoadInfo. Given Comment 5 it's probably the better solution in the end.

Comment 8

[Honza Bambas (:mayhemer)]

Yeah, that sounds like a strong counter argument.  I think we've already had a patch close to that solution once.

So, you need to add the target principal best in HttpBaseChannel::SetupReplacementChannel (a single place), somewhere around [1].  Removal should be done in !succeeded case at [2] which is the single place we go through after all the pre-redirect dance is done (either succeeded or failed).  You will actually invert the condition and behavior of the code you have added there recently.  No big science.

On child I don't think there is a place we call after pre-redirect dance is done in the failure case.  We have one, but only for the succeeded case.  Hence, you will have to enhance http ipdl at [3] to send down a 'bool success' flag.  Called from HttpChannelParent::CompleteRedirect.  To make it more clear, do Redirect3Complete(bool succeeded).

[1] http://hg.mozilla.org/mozilla-central/annotate/095988abdc56/netwerk/protocol/http/HttpBaseChannel.cpp#l2289
[2] http://hg.mozilla.org/mozilla-central/annotate/095988abdc56/netwerk/protocol/http/nsHttpChannel.cpp#l210
[3] http://hg.mozilla.org/mozilla-central/annotate/095988abdc56/netwerk/protocol/http/PHttpChannel.ipdl#l126

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1194052_redirectchainincludinginternalredirects.patch

Part1: Adding a second Array of redirect principals to loadInfo so we have one array that includes internal redirects and one that does *not* contain internal redirects.

Comment 10

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1194052_redirectchain.patch

Part2: Appending to that redirectChain only in HttpBaseChannel.cpp and popping the last element from the redirectchain in case of a fallback.

Honza, thanks for your guidance, but I do have a few more questions:

1) In your previous comment you mentioned we should only append to the redirectChain within HttpBaseChannel::SetupReplacementChannel(). Does that mean we have to give up on the BaseChannel case which we have currently? What would it need to keep that BaseChannel case as well?

2) As started in Part1 (the other patch) we would like to have *2* redirectChains within the loadInfo. One that includes internal redirects and one that does *not*. Is there an easy way to figure within SetupReplacementChannel whether we are going through an internal redirect or an external one? If not, then my suggestion would have been to pass a boolean flag to SetupReplacementChannel which would be set to true if either REDIRECT_INTERNAL or REDIRECT_STS_UPGRADE is present within the flags. Does that makes sense?

3) Within ReportRedirectResult() as well as within Redirect3Complete() we would also have to know whether an internal or an external redirect did not succeed. Any suggestions how we could accomplish that?

Comment 11

[Honza Bambas (:mayhemer)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #10)
> Created attachment 8651265 [details] [diff] [review]
> bug_1194052_redirectchain.patch
> 
> Part2: Appending to that redirectChain only in HttpBaseChannel.cpp and
> popping the last element from the redirectchain in case of a fallback.
> 
> Honza, thanks for your guidance, but I do have a few more questions:
> 
> 1) In your previous comment you mentioned we should only append to the
> redirectChain within HttpBaseChannel::SetupReplacementChannel(). Does that
> mean we have to give up on the BaseChannel case which we have currently?
> What would it need to keep that BaseChannel case as well?

I never said that.  You need both.  http channel is in no way bound to base channel.  your question where only about http channel.

> 
> 2) As started in Part1 (the other patch) we would like to have *2*
> redirectChains within the loadInfo. One that includes internal redirects and
> one that does *not*. Is there an easy way to figure within
> SetupReplacementChannel whether we are going through an internal redirect or
> an external one? 

rather pass whole 'flags' to that method.

> If not, then my suggestion would have been to pass a
> boolean flag to SetupReplacementChannel which would be set to true if either
> REDIRECT_INTERNAL or REDIRECT_STS_UPGRADE is present within the flags. Does
> that makes sense?
> 
> 3) Within ReportRedirectResult() as well as within Redirect3Complete() we
> would also have to know whether an internal or an external redirect did not
> succeed. Any suggestions how we could accomplish that?

have a flag on the http base channel (please, a bitfield).  you can then use it both methods to discover the nature of the redirect.  you may expose any methods to set the info on the channel on nsIHttpChannelInternal interface

Comment 12

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1194052_redirectchainincludinginternalredirects.patch

Comment 13

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1194052_redirectchain.patch

Honza, can you please review that patch? The only thing I am not sure is if there is  also a fallback case for nsBaseChannel where we have to remove the principal form the redirectChain. If so, where would we have to do that?

Comment 14

[Honza Bambas (:mayhemer)]

Comment on attachment 8654383 [details] [diff] [review]
bug_1194052_redirectchain.patch

Review of attachment 8654383 [details] [diff] [review]:
-----------------------------------------------------------------

I'd like to see a better version.

just to confirm - each channel initially has its own instance of LoadInfo?

::: netwerk/base/LoadInfo.cpp
@@ +308,5 @@
> +{
> +  MOZ_ASSERT(mRedirectChainIncludingInternalRedirects.Length() > 0,
> +             "can not pop element from empty array");
> +  NS_ENSURE_TRUE(mRedirectChainIncludingInternalRedirects.Length() > 0,
> +                 NS_ERROR_UNEXPECTED);

there also should be an assertion about a thread this is called on (always when you manipulate any of the mRedirectChain* arrays).  I presume this will always be called only on the main thread?

::: netwerk/base/nsILoadInfo.idl
@@ +396,5 @@
>                                   in boolean isInternalRedirect);
>  
> +  /*
> +   * Remove the last element from the redirectChain.
> +   * Used to remove elements in case the redirect does not succeed.

add a note about the isInternalRedirect arg that it has to correspond with the one passed to appendRedirectedPrincipal

::: netwerk/protocol/http/HttpChannelChild.cpp
@@ +1142,5 @@
>  class Redirect3Event : public ChannelEvent
>  {
>   public:
>    explicit Redirect3Event(HttpChannelChild* child) : mChild(child) {}
> +  void Run() { mChild->Redirect3Complete(true); }

this is bad

@@ +1153,4 @@
>  {
>    LOG(("HttpChannelChild::RecvRedirect3Complete [this=%p]\n", this));
>    if (mEventQ->ShouldEnqueue()) {
>      mEventQ->Enqueue(new Redirect3Event(this));

you must arm the Redirect3Event with the 'succeeded' arg, right?

@@ +1244,4 @@
>      if (mLoadInfo) {
> +      bool isInternalRedirect =
> +        (mLoadFlags & nsIChannelEventSink::REDIRECT_INTERNAL) ||
> +        (mLoadFlags & nsIChannelEventSink::REDIRECT_STS_UPGRADE);

loadflags??

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +207,5 @@
>          return;
>  
> +    // if the redirect did not succeed then we have to remove
> +    // the principal from the redirectchain within the loadInfo.
> +    if (!succeeded && mChannel->mLoadInfo) {

please make sure we don't remove mLoadInfo between where you add the principal and this point.

@@ +2744,5 @@
>      rv = gHttpHandler->NewChannel2(mURI,
>                                     mLoadInfo,
>                                     getter_AddRefs(newChannel));
>      NS_ENSURE_SUCCESS(rv, rv);
> +    uint32_t redirectFlags = nsIChannelEventSink::REDIRECT_INTERNAL;

blank line before this line

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8654374 [details] [diff] [review]
bug_1194052_redirectchainincludinginternalredirects.patch

Review of attachment 8654374 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, but honza should verify that the HttpChannel(Child).cpp changes are correct. I.e. that all of them should pass 'false'

Comment 16

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Jonas Sicking (:sicking) from comment #15)
> Comment on attachment 8654374 [details] [diff] [review]
> bug_1194052_redirectchainincludinginternalredirects.patch
> 
> Review of attachment 8654374 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me, but honza should verify that the HttpChannel(Child).cpp changes are
> correct. I.e. that all of them should pass 'false'

This is just an artifact of how I structured the two patches. This patch you just reviewed does the groundwork, whereas the other patch that Honza reviews will actually pass true/false depending on wehther it's an internal or an external redirect.

Comment 17

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

cool, r=me then.

Comment 18

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1194052_redirectchain.patch

(In reply to Honza Bambas (not reviewing) (:mayhemer) from comment #14)
> just to confirm - each channel initially has its own instance of LoadInfo?

We attach a loadInfo to every newly created channel which is propagated during redirects.

> >      mEventQ->Enqueue(new Redirect3Event(this));
> 
> you must arm the Redirect3Event with the 'succeeded' arg, right?

Yes, that makes sense.
 
> @@ +1244,4 @@
> >      if (mLoadInfo) {
> > +      bool isInternalRedirect =
> > +        (mLoadFlags & nsIChannelEventSink::REDIRECT_INTERNAL) ||
> > +        (mLoadFlags & nsIChannelEventSink::REDIRECT_STS_UPGRADE);

Obviously redirectflags. Similar to nsBaseChannel I also added a member 'mRedirectFlags' to HttpBaseChannel. This should work because SetupReplacementChannel is called before the first use.

> > +    // if the redirect did not succeed then we have to remove
> > +    // the principal from the redirectchain within the loadInfo.
> > +    if (!succeeded && mChannel->mLoadInfo) {
> 
> please make sure we don't remove mLoadInfo between where you add the
> principal and this point.

I manually verified, or do you want any assertions for that? If so, how and where?

Comment 19

[Honza Bambas (:mayhemer)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #18)
> > >      mEventQ->Enqueue(new Redirect3Event(this));
> > 
> > you must arm the Redirect3Event with the 'succeeded' arg, right?
> 
> Yes, that makes sense.

That's a pretty serious omission and bug if you don't ;)

>  
> > @@ +1244,4 @@
> > >      if (mLoadInfo) {
> > > +      bool isInternalRedirect =
> > > +        (mLoadFlags & nsIChannelEventSink::REDIRECT_INTERNAL) ||
> > > +        (mLoadFlags & nsIChannelEventSink::REDIRECT_STS_UPGRADE);
> 
> Obviously redirectflags. Similar to nsBaseChannel I also added a member
> 'mRedirectFlags' to HttpBaseChannel. This should work because
> SetupReplacementChannel is called before the first use.

To be sure I follow your answer - you are about to replace mLoadFlags with mRedirectFlags here, right?  Since load flags has absolutely nothing to do with redirect flags.  Typical problem when you don't use non-typed enums (impossible to change it here tho)

> 
> > > +    // if the redirect did not succeed then we have to remove
> > > +    // the principal from the redirectchain within the loadInfo.
> > > +    if (!succeeded && mChannel->mLoadInfo) {
> > 
> > please make sure we don't remove mLoadInfo between where you add the
> > principal and this point.
> 
> I manually verified, or do you want any assertions for that? If so, how and
> where?

Hmm.. ideal would be to have a debug only flag (initially false) that will rise to true when you successfully push a principal to the redirect chain (i.e. when mLoadInfo was not null).  then when you want to remove the principal, do the following:
- when mLoadInfo is non-null, assert your flag is true
- when mLoadInfo is null, assert your flag is false
(simply: assert(!!mLoadInfo == mHasSetRedirectPrincipal))

also would be nice (if not overcomplicated) to check the principal you are removing is the principal you expect to be removed.  but that is really just a nice to have, definitely not needed until we encounter some issues around that.

Comment 20

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1194052_redirectchain.patch

(In reply to Honza Bambas (not reviewing) (:mayhemer) from comment #19)
> To be sure I follow your answer - you are about to replace mLoadFlags with
> mRedirectFlags here, right?  Since load flags has absolutely nothing to do
> with redirect flags.  Typical problem when you don't use non-typed enums
> (impossible to change it here tho)

Throughout the patch we have to rely on redirectFlags and *not* loadflags, it's unfortunate that both flags are a uint32_t so the compiler can't distinguish between the two. To answer your question, yes we should use the newly introduced mRedirectFlags rather than mLoadFlags. To be more explicit (at least from a semantic point of view) I am using the variable name 'redirectFlags' rather than just 'flags'. Same for passing the redirect flags around as arguments.

> Hmm.. ideal would be to have a debug only flag (initially false) that will
> rise to true when you successfully push a principal to the redirect chain
> (i.e. when mLoadInfo was not null).

I am not sure I get the full idea behind that assertion, because what if I replace the LoadInfo with a different LoadInfo in between? But ok, I added what you requested.

Either way, this should be ready for review now.

Comment 21

[Honza Bambas (:mayhemer)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #20)
> Created attachment 8656827 [details] [diff] [review]
> > Hmm.. ideal would be to have a debug only flag (initially false) that will
> > rise to true when you successfully push a principal to the redirect chain
> > (i.e. when mLoadInfo was not null).
> 
> I am not sure I get the full idea behind that assertion, because what if I
> replace the LoadInfo with a different LoadInfo in between? 

I presume it's not possible to change LoadInfo during the channel's lifetime.

I'll try to take a look next week at this patch.

Comment 22

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

> I presume it's not possible to change LoadInfo during the channel's lifetime.

Yes, we definitely don't want to switch LoadInfo instance for a channel.

If this thing of adding the redirect and then trying to remove it is getting too complicated, I can live with that we create a new LoadInfo instance for the new, redirected, channel.

Comment 23

[Honza Bambas (:mayhemer)]

Comment on attachment 8656827 [details] [diff] [review]
bug_1194052_redirectchain.patch

Review of attachment 8656827 [details] [diff] [review]:
-----------------------------------------------------------------

First and major omission: missing a test, also a wrap for ipc.  see network/test/unit(_ipc)/test_redirect*.js for examples.

::: netwerk/base/LoadInfo.cpp
@@ +311,5 @@
> +  MOZ_ASSERT(NS_IsMainThread());
> +  MOZ_ASSERT(mRedirectChainIncludingInternalRedirects.Length() > 0,
> +             "can not pop element from empty array");
> +  NS_ENSURE_TRUE(mRedirectChainIncludingInternalRedirects.Length() > 0,
> +                 NS_ERROR_UNEXPECTED);

you must check both your arrays.

::: netwerk/base/nsBaseChannel.cpp
@@ +172,5 @@
>      }
>      else {
>        rv = mRedirectChannel->AsyncOpen(mListener, mListenerContext);
>      }
>      NS_ENSURE_SUCCESS(rv, rv);

you should probably remove it on a failure here, right?

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +2294,5 @@
>        newPBChannel->SetPrivate(mPrivateBrowsing);
>      }
>    }
>  
> +  mRedirectFlags = redirectFlags;

probably set at the top of the method to be sure it has been populated?

@@ +2301,5 @@
> +    bool isInternalRedirect =
> +      (mRedirectFlags & nsIChannelEventSink::REDIRECT_INTERNAL) ||
> +      (mRedirectFlags & nsIChannelEventSink::REDIRECT_STS_UPGRADE);
> +    mLoadInfo->AppendRedirectedPrincipal(GetURIPrincipal(), isInternalRedirect);
> +    newChannel->SetLoadInfo(mLoadInfo);

hmm.. if !mLoadInfo, don't we want to overwrite (nullify) any LoadInfo that may happen to be currently on the new channel?  that is how it works w/o this patch.  we should not change it.

::: netwerk/protocol/http/HttpBaseChannel.h
@@ +390,5 @@
>    // Current suspension depth for this channel object
>    uint32_t                          mSuspendCount;
>  
>    nsCOMPtr<nsIURI>                  mAPIRedirectToURI;
> +

why this ws?

@@ +397,5 @@
> +  // redirectflags and additional debug flag to make sure we appended and popped
> +  // redirect information to and from the right loadInfo.
> +  uint32_t                          mRedirectFlags;
> +#if DEBUG
> +  bool                              mHasSetRedirectPrincipal;

use mozilla::DebugOnly

::: netwerk/protocol/http/HttpChannelChild.cpp
@@ +1252,5 @@
>                                                        mListenerContext);
>  
>    // Redirecting to new channel: shut this down and init new channel
>    if (mLoadGroup)
>      mLoadGroup->RemoveRequest(this, nullptr, NS_BINDING_ABORTED);

this all code must run only in the aSucceeded == true case, right?

@@ +1255,5 @@
>    if (mLoadGroup)
>      mLoadGroup->RemoveRequest(this, nullptr, NS_BINDING_ABORTED);
>  
> +  if (NS_FAILED(rv) || !aSucceeded) {
> +    NS_WARNING("CompleteRedirectSetup failed, HttpChannelChild already open?");

this warning must trigger only in the NS_FAILED(rv) case

::: netwerk/protocol/http/HttpChannelParent.cpp
@@ +1073,5 @@
>         this, succeeded));
>  
>    if (succeeded && !mIPCClosed) {
>      // TODO: check return value: assume child dead if failed
> +    unused << SendRedirect3Complete(succeeded);

ehm... you need to send this even in the !succeeded case, right?

tests would catch this.

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +194,5 @@
>  
>        mChannel->mHasAutoRedirectVetoNotifier = true;
>      }
> +    ~AutoRedirectVetoNotifier() { ReportRedirectResult(false, mRedirectFlags); }
> +    void RedirectSucceeded() { ReportRedirectResult(true, mRedirectFlags); }

unnecessary changes.  this class has access (clearly) to mRedirectFlags on the channel.  use it.

@@ +215,5 @@
> +               "what happenend to the loadInfo and it's redirectchain?");
> +    if (!succeeded && mChannel->mLoadInfo) {
> +        bool isInternalRedirect =
> +         (redirectFlags & nsIChannelEventSink::REDIRECT_INTERNAL) ||
> +         (redirectFlags & nsIChannelEventSink::REDIRECT_STS_UPGRADE);

nit: !!(redirectFlags & (nsIChannelEventSink::REDIRECT_INTERNAL | nsIChannelEventSink::REDIRECT_STS_UPGRADE))

Comment 24

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1194052_redirectchain_tests.patch

(In reply to Honza Bambas (not reviewing) (:mayhemer) from comment #23)
> First and major omission: missing a test, also a wrap for ipc.  see
> network/test/unit(_ipc)/test_redirect*.js for examples.

Agreed, here is a mochitest so we can also test internal redirects by using CPSs upgrade-inescure-requests directive. Please note there is also test_redirect_history which also has a wrap for ipc in addition to my newly added tests.

Comment 25

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1194052_redirectchain.patch

(In reply to Honza Bambas (not reviewing) (:mayhemer) from comment #23)
> Comment on attachment 8656827 [details] [diff] [review]
> > +  NS_ENSURE_TRUE(mRedirectChainIncludingInternalRedirects.Length() > 0,
> > +                 NS_ERROR_UNEXPECTED);
> 
> you must check both your arrays.

I think am doing that already, no?

> ::: netwerk/base/nsBaseChannel.cpp
> >        rv = mRedirectChannel->AsyncOpen(mListener, mListenerContext);
> >      }
> >      NS_ENSURE_SUCCESS(rv, rv);
> 
> you should probably remove it on a failure here, right?

Good catch, done!

> ::: netwerk/protocol/http/HttpBaseChannel.cpp
> > +  mRedirectFlags = redirectFlags;
> 
> probably set at the top of the method to be sure it has been populated?

There is no return before that assignment in that function. I think we can leave that.

> @@ +2301,5 @@
> > +    newChannel->SetLoadInfo(mLoadInfo);
> 
> hmm.. if !mLoadInfo, don't we want to overwrite (nullify) any LoadInfo that
> may happen to be currently on the new channel?  that is how it works w/o
> this patch.  we should not change it.

In fact that should never happen, but I agree, we should not change behavior here.

> @@ +397,5 @@
> > +#if DEBUG
> > +  bool                              mHasSetRedirectPrincipal;
> 
> use mozilla::DebugOnly

Oh, learned something today - thanks!

> ::: netwerk/protocol/http/HttpChannelChild.cpp
> @@ +1252,5 @@
> >                                                        mListenerContext);
> >  
> >    // Redirecting to new channel: shut this down and init new channel
> >    if (mLoadGroup)
> >      mLoadGroup->RemoveRequest(this, nullptr, NS_BINDING_ABORTED);
> 
> this all code must run only in the aSucceeded == true case, right?

I don't think so, we are popping a principal from the redirectchain if !aSucceeded, no?

> ::: netwerk/protocol/http/HttpChannelParent.cpp
> @@ +1073,5 @@
> >         this, succeeded));
> >  
> >    if (succeeded && !mIPCClosed) {
> >      // TODO: check return value: assume child dead if failed
> > +    unused << SendRedirect3Complete(succeeded);
> 
> ehm... you need to send this even in the !succeeded case, right?

I don't know what you mean? Send what?

> @@ +215,5 @@
> > +               "what happenend to the loadInfo and it's redirectchain?");
> > +    if (!succeeded && mChannel->mLoadInfo) {
> > +        bool isInternalRedirect =
> > +         (redirectFlags & nsIChannelEventSink::REDIRECT_INTERNAL) ||
> > +         (redirectFlags & nsIChannelEventSink::REDIRECT_STS_UPGRADE);
> 
> nit: !!(redirectFlags & (nsIChannelEventSink::REDIRECT_INTERNAL |
> nsIChannelEventSink::REDIRECT_STS_UPGRADE))

Two negations are way more confusing then an 'or' in my opinion. If you insist I am going to change that, otherwise I would like to keep it that way.

Thanks for the reviews!

Comment 26

[Honza Bambas (:mayhemer)]

Comment on attachment 8666230 [details] [diff] [review]
bug_1194052_redirectchain.patch

Review of attachment 8666230 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/base/nsBaseChannel.cpp
@@ +172,5 @@
>      }
>      else {
>        rv = mRedirectChannel->AsyncOpen(mListener, mListenerContext);
>      }
> +    if (NS_FAILED(rv)) {

if (NS_WARN_IF(NS_FAILED(rv))

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +2447,5 @@
>        newPBChannel->SetPrivate(mPrivateBrowsing);
>      }
>    }
>  
> +  mRedirectFlags = redirectFlags;

set this at the top of the method regardless there are no returns before this point, please.

::: netwerk/protocol/http/HttpChannelChild.cpp
@@ +1273,3 @@
>  
>    // Release ref to new channel.
>    mRedirectChannelChild = nullptr;

Redirect3Complete() didn't run at all when |aSucceeded == false|.

please fix the code accordingly.

::: netwerk/protocol/http/HttpChannelParent.cpp
@@ +1295,5 @@
>         this, succeeded));
>  
>    if (succeeded && !mIPCClosed) {
>      // TODO: check return value: assume child dead if failed
> +    unused << SendRedirect3Complete(succeeded);

:) you really don't see it?

succeeded will always be |true| here.  but you must call SendRedirect3Complete ALWAYS.

so, the code has to be:

if (!mIPCClosed) {
  SendRedirect3Complete(succeeded);
}

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +200,5 @@
>      void RedirectSucceeded() {ReportRedirectResult(true);}
>  
>  private:
>      nsHttpChannel* mChannel;
> +    uint32_t mRedirectFlags;

no need to store the flags in this class as a member.  just do mChannel->mRedirectFlags below.

@@ +217,5 @@
> +               "what happenend to the loadInfo and it's redirectchain?");
> +    if (!succeeded && mChannel->mLoadInfo) {
> +        bool isInternalRedirect =
> +         (mRedirectFlags & nsIChannelEventSink::REDIRECT_INTERNAL) ||
> +         (mRedirectFlags & nsIChannelEventSink::REDIRECT_STS_UPGRADE);

do (mChannel->mRedirectFlags & (nsIChannelEventSink::REDIRECT_INTERNAL | nsIChannelEventSink::REDIRECT_STS_UPGRADE))

applies to more places in the patch

you are right there is no need for !! to convert to strict bool

Comment 27

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1194052_redirectchain.patch

Thanks Honza, here is an updated patch with all comments addressed. SendRedirect3Complete() should now run even if |succeeded = false|.

Comment 28

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8666227 [details] [diff] [review]
bug_1194052_redirectchain_tests.patch

Honza, things have changed again and it seems we are not going to need that patch in the end. Clearing the ni? and r? flag for now so are not wasting time reviewing.

Once I know more about the new direction, I will post an update to that bug again.

Comment 29

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1194052_part1_add_internal_redirectchain.patch

Alrighty, this one has been sitting around for quite some time now. Let's make some progress. Just rebased this patch, carrying over r+ from Jonas.

Comment 30

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1194052_part2_fix_loadinfo_clone_functionality.patch

Since we are now going to make a copy of the loadinfo for redirects, we have to fix the copy constructor first.

Comment 31

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1194052_part3_append_to_redirectchain_before_asyncopen.patch

So, the parts where we append to the redirectchain remain mostly the same, but I elminated the parts of the fallback since we are now creating a copy of the loadInfo for redirects. Once you f+ it, I will flag Honza for review.

Comment 32

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1194052_tests.patch

Just rebased the tests. Honza will (hopefully) review them.

Comment 33

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8681062 [details] [diff] [review]
bug_1194052_part3_append_to_redirectchain_before_asyncopen.patch

Review of attachment 8681062 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/protocol/http/HttpChannelChild.cpp
@@ +1249,5 @@
>                                        const nsHttpResponseHead* responseHead)
>  {
>    LOG(("HttpChannelChild::BeginNonIPCRedirect [this=%p]\n", this));
>  
> +  uint32_t redirectFlags = nsIChannelEventSink::REDIRECT_INTERNAL;

Nit: I'd just use nsIChannelEventSink::REDIRECT_INTERNAL in both places.

At the very least use "const uint32_t redirectFlags..."

Comment 34

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8681061 [details] [diff] [review]
bug_1194052_part2_fix_loadinfo_clone_functionality.patch

Review of attachment 8681061 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/base/LoadInfo.cpp
@@ +96,5 @@
>    , mOuterWindowID(rhs.mOuterWindowID)
>    , mParentOuterWindowID(rhs.mParentOuterWindowID)
> +  , mEnforceSecurity(rhs.mEnforceSecurity)
> +  , mInitialSecurityCheckDone(rhs.mInitialSecurityCheckDone)
> + , mOriginAttributes(rhs.mOriginAttributes)

Fix indentation

@@ +104,5 @@
> +      rhs.mRedirectChainIncludingInternalRedirects[i]);
> +  }
> +  for (uint32_t i = 0; i < rhs.mRedirectChain.Length(); i++) {
> +    mRedirectChain.AppendElement(rhs.mRedirectChain[i]);
> +  }

Simply add

, mRedirectChainIncludingInternalRedirects(rhs.mRedirectCh...)
, mRedirectChain(rhs.mRedirectChain)

to the initializer list

@@ +159,5 @@
> +LoadInfo::CloneAndResetInitialSecurityChecksDone() const
> +{
> +  RefPtr<LoadInfo> copy(new LoadInfo(*this));
> +  copy->mEnforceSecurity = false;
> +  copy->mInitialSecurityCheckDone = false;

You need to also clear the redirect chains.

It might be more descriptive to call this "CloneForNewRequest" or some such.

::: netwerk/base/LoadInfo.h
@@ +57,3 @@
>    already_AddRefed<nsILoadInfo> Clone() const;
> +  // create a copy of the loadinfo but reset
> +  // mEnforceSecurity and mInitialSecurityCheckDone

Might be better to say something like:

"Creates a copy of the loadinfo which is appropriate to use for a separate request. I.e. not for a redirect or an inner channel, but when a separate request is made with the same security properties."

Comment 35

[Honza Bambas (:mayhemer)]

Comment on attachment 8681062 [details] [diff] [review]
bug_1194052_part3_append_to_redirectchain_before_asyncopen.patch

Review of attachment 8681062 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/base/nsBaseChannel.cpp
@@ +103,5 @@
> +      (redirectFlags & (nsIChannelEventSink::REDIRECT_INTERNAL |
> +                        nsIChannelEventSink::REDIRECT_STS_UPGRADE));
> +    newLoadInfo->AppendRedirectedPrincipal(uriPrincipal, isInternalRedirect);
> +    newChannel->SetLoadInfo(newLoadInfo);
> +  }

one thing (I was already pointing at that in a different patch for this functionality) is that the unpatched code does replace any load info happen to be set on the new channel even when the old channel's load info is null.  you are breaking this.  Not sure what impact that may have, but I'd rather keep it.

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +2500,5 @@
> +  // make a copy of the loadinfo, append to the redirectchain
> +  // and set it on the new channel
> +  if (mLoadInfo) {
> +    nsCOMPtr<nsILoadInfo> newLoadInfo =
> +      static_cast<mozilla::LoadInfo*>(mLoadInfo.get())->Clone();

oh, it's bad that Clone() is not on nsILoadInfo.  I personally don't like static_casts.  You never know if this is replaced with a different class, despite it being built-in.

Comment 36

[Honza Bambas (:mayhemer)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #32)
> Created attachment 8681063 [details] [diff] [review]
> bug_1194052_tests.patch
> 
> Just rebased the tests. Honza will (hopefully) review them.

If you want a review, just ask via bugzilla, no?

Comment 37

[Honza Bambas (:mayhemer)]

Comment on attachment 8681063 [details] [diff] [review]
bug_1194052_tests.patch

Review of attachment 8681063 [details] [diff] [review]:
-----------------------------------------------------------------

I assume this could not be made as an xpcshell test because we need to enforce CPS, right?

Please fix the formatting according my comment below before this lands!

::: netwerk/test/mochitests/file_loadinfo_redirectchain.sjs
@@ +62,5 @@
> +  if (queryString == "redir-0" ||
> +      queryString == "redir-https-0") {
> +	  response.setHeader("Content-Type", "text/html", false);
> +  	response.write("checking redirectchain");
> + 	  return;

please fix indention (may apply to the whole patch)

remember that we use 2 spaces indention and don't allow tabs.  tho i see them in this file...

Comment 38

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1194052_part2_fix_loadinfo_clone_functionality.patch

Incorporated Jonas' suggestions. Carrying over r+.

Comment 39

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1194052_part3_append_to_redirectchain_before_asyncopen.patch

Honza, I see that you are out till November 19th, any chance you can sign off on this patch or are you fine with Jonas r+?

(In reply to Honza Bambas, off till Nov 19 (:mayhemer) from comment #35)
> one thing (I was already pointing at that in a different patch for this
> functionality) is that the unpatched code does replace any load info happen
> to be set on the new channel even when the old channel's load info is null. 
> you are breaking this.  Not sure what impact that may have, but I'd rather
> keep it.

I thought about this for a while, and you are absolutely right. I added a comment on top.
 
> oh, it's bad that Clone() is not on nsILoadInfo.  I personally don't like
> static_casts.  You never know if this is replaced with a different class,
> despite it being built-in.

Chatted with Jonas, we are considering converting nsILoadInfo into a webidl, but would like to keep what we have for now.

Comment 40

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1194052_tests.patch

(In reply to Honza Bambas, off till Nov 19 (:mayhemer) from comment #36)

> > Just rebased the tests. Honza will (hopefully) review them.
> If you want a review, just ask via bugzilla, no?

Yes, I would have flagged you for review. The message was meant for Jonas so that he knows he doesn't have to look at the tests. I actually wanted to save you some cycles and wait till Jonas is fine with patches within that bug before I was asking you for review.

Anyway, thanks for the speedy response and review.

Carrying over r+ from mayhemer.

Comment 41

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8681401 [details] [diff] [review]
bug_1194052_part3_append_to_redirectchain_before_asyncopen.patch

As clarified with Honza over email, he just forgot to set the flag to r+ for this patch within Comment 35.

Comment 42

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/70128b51c777
https://hg.mozilla.org/integration/mozilla-inbound/rev/eab20c4430c6
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9e071ff7308
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b323c589080

Comment 43

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/70128b51c777
https://hg.mozilla.org/mozilla-central/rev/eab20c4430c6
https://hg.mozilla.org/mozilla-central/rev/d9e071ff7308
https://hg.mozilla.org/mozilla-central/rev/9b323c589080

Comment 44

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/70128b51c777
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/eab20c4430c6
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/d9e071ff7308
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/9b323c589080

Comment 45

[Carsten Book [:Tomcat]]

removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion