974018 - implement nsIRedirectChannel on nsHttpChannel

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[[:mmc] Monica Chew (no longer reading bugmail)]

See https://wiki.mozilla.org/Security/Features/Application_Reputation_Design_Doc

I started working on this then realized I never filed a bug!

Comment 1

[Honza Bambas (:mayhemer)]

Can you please explain why this is needed for your project?  I am not sure exposing this is the right thing, but maybe I just need some more context here.

Comment 2

[[:mmc] Monica Chew (no longer reading bugmail)]

Hi Honza,

Please see https://wiki.mozilla.org/Security/Features/Application_Reputation_Design_Doc#High-level_overview. In Chrome, the redirects are used to check downloaded binaries against local allowlists and blocklists. If a match is found on the blocklist, the file is not saved to disk. If a match is found on the allowlist, we get to skip the remote lookup part of this check.

Thanks,
Monica

Comment 3

[[:mmc] Monica Chew (no longer reading bugmail)]

It turns out that mRedirectedCachekeys is not getting populated anyway. It is only used for cached redirects and initialized until nsHttpChannel::OnCacheEntryCheck.

The easiest place to hook in to keep track of redirects is probably in AsyncProcessRedirection, then maybe copied over in SetupReplacementChannel. However, I don't totally understand everything that's going on here.

I think the best thing to do is to not make this block 933432, but just pass the referrer back to application reputation query and hope that's good enough.

Comment 4

[Honza Bambas (:mayhemer)]

Yes, I thought so.  I think we need to implement something totally new for this.  Other option is to hook to the redirect veto general callbacks and track redirects for this purpose externally.

I think there is a single place you can register a global redirect observer, but I need to remember how to do this...

Comment 5

[[:mmc] Monica Chew (no longer reading bugmail)]

(In reply to Honza Bambas (:mayhemer) from comment #4)
> Yes, I thought so.  I think we need to implement something totally new for
> this.  Other option is to hook to the redirect veto general callbacks and
> track redirects for this purpose externally.
> 
> I think there is a single place you can register a global redirect observer,
> but I need to remember how to do this...

Is it this?

http://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsIChannelEventSink.idl#90

Comment 6

[Honza Bambas (:mayhemer)]

Yes, that is the interface, but your object implementing it must be registered to actually be invoked.  I am not sure where it is right now..

Note: ignore redirects flagged with REDIRECT_INTERNAL.

Comment 7

[Honza Bambas (:mayhemer)]

Jason, don't you remember how to globally observe redirects and call to your own nsIChannelEventSink.asyncOnChannelRedirect for every channel redirect?  

I know add-ons can somehow easily do this...

Comment 8

[[:mmc] Monica Chew (no longer reading bugmail)]

Attachment: pass-redirects

Btw, for my purposes I don't think I can use a global observer, I need the redirects associated with a particular channel in DownloadCore.jsm.

Comment 9

[Jason Duell]

I haven't traced back the logic for the global redirect observer completely, but it looks like nsScriptSecurityManager registers itself as a service, so presumably any global observers are done through it somehow?

   http://mxr.mozilla.org/mozilla-central/search?string=NS_GLOBAL_CHANNELEVENTSINK_CONTRACTID

Like mmc I'm not sure the global route is the easiest or most efficient, but it might be ok.

nsIRedirectResultListener does seem like it might work for this, but right now you have to be the callbacks in order to get that notification.  We could change that to have a list of listeners hang off the channel?

Comment 10

[Jason Duell]

Or an API that ops-in a channel into keeping a list of its URI/redirect chain, to be queried at any point from OnStartRequest until ~destruction.

Comment 11

[Jason Duell]

Thinking about it for 2 seconds more, I think comment 10 is better--the fewer observers we have to support the better (they junk up our ability to operate on different threads, etc.)

Comment 12

[Honza Bambas (:mayhemer)]

mmc, do you want us to work on this bug?

Comment 13

[[:mmc] Monica Chew (no longer reading bugmail)]

(In reply to Honza Bambas (:mayhemer) from comment #12)
> mmc, do you want us to work on this bug?

mayhemer, that would be GREAT!

Comment 14

[[:mmc] Monica Chew (no longer reading bugmail)]

Hey Honza,

We ran some preliminary tests on the application reputation feature last week, using a corpus of 600+ malware binaries from NSS Labs. The verdict was that application reputation should reduce undetected malware by about half, but Chrome still does better. I think a big part of that reason is that we aren't checking the redirect chain against the blocklists. I'd like to get this as part of FF 31. Does that look feasible?

Thanks,
Monica

https://wiki.mozilla.org/Security/Features/Application_Reputation/Preliminary_Results

Comment 15

[[:mmc] Monica Chew (no longer reading bugmail)]

also Jonas popped into the team meeting today to ask for a similar API (principals associated with redirects on nsIChannel)

Comment 16

[Honza Bambas (:mayhemer)]

(In reply to Monica Chew [:mmc] (please use needinfo) from comment #15)
> also Jonas popped into the team meeting today to ask for a similar API
> (principals associated with redirects on nsIChannel)

I'll talk to him first.  I'm right now occupied with the new http cache that is closing to get into the tree.  But I can find some time to do this.

Comment 17

[[:mmc] Monica Chew (no longer reading bugmail)]

Attachment: Pass redirects on nsIChannel where available (

Comment 18

[[:mmc] Monica Chew (no longer reading bugmail)]

Attachment: Pass redirects on nsIChannel where available (

Comment 19

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 8404949 [details] [diff] [review]
Pass redirects on nsIChannel where available (

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

Hi,

This patch is not ready for review because it leaks.

This passes:
./mach xpcshell-test netwerk/test/unit/test_redirect-caching_passing.js 

And this fails because it's leaking redirect URLs:
./mach xpcshell-test netwerk/test/unit_ipc/test_redirect-caching_passing_wrap.js

With NSPR_LOG_MODULES=nsHttp:5, it seems that nsHttpChannel dtor is being called in the regular unittest but not the unit_ipc one. Any clues?

Thanks,
Monica

Comment 20

[[:mmc] Monica Chew (no longer reading bugmail)]

Attachment: logs from leaky test

Comment 21

[[:mmc] Monica Chew (no longer reading bugmail)]

Attachment: logs from non-leaky test

Comment 22

[Patrick McManus [:mcmanus]]

jason is the necko e10s expert.. maybe he'll see something quickly

Comment 23

[[:mmc] Monica Chew (no longer reading bugmail)]

nm, I think I found it. There must be a name for realizing something shortly after asking for help... :)

Comment 24

[[:mmc] Monica Chew (no longer reading bugmail)]

Attachment: Pass redirects on nsIChannel where available (

Comment 25

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 8405095 [details] [diff] [review]
Pass redirects on nsIChannel where available (

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

Hi Honza,

1) I didn't follow Jason's suggestion about flagging this off because in my use case, you can't tell whether or not a URL is going to reach a download at the start of any redirect chain.
2) GetPrincipal() was null for most of the time, so I didn't implement nsIChannel.redirects as an nsIArray of nsIPrincipals, instead it is an array of nsIURIs.

There is more testing to be done, but I figured I'd get your feedback on the interface first.

Thanks,
Monica

Comment 26

[[:mmc] Monica Chew (no longer reading bugmail)]

Btw, the link to Jonas's proposal (to include nsIPrincipals on nsIChannels, among other things) is here: https://etherpad.mozilla.org/BetterNeckoSecurityHooks

Comment 27

[Honza Bambas (:mayhemer)]

Comment on attachment 8405095 [details] [diff] [review]
Pass redirects on nsIChannel where available (

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

thanks for the patch.  sorry to say that, but I think a necko peer should rather do this.  please read on.

I'm asking for f? from Patrick, since this is a huge API change and will break most of the extensions.  Personally I'm not happy to change nsIChannel just to expose a redirect chain and force every implementer of nsIChannel to deal with it.  I'd rather do this via monitoring of the redirect chain by callbacks as I originally suggested.

Other option that crosses my mind is to have some service or global in net/protocol/http that will collect the redirects (by manual calls from nsHttpChannel) where you can pick it up by providing the final channel.  Every time a channel is successfully redirected, you can register the channel+URI in this service that will automatically collect the chain.

in general to the patch:

- when you don't change an interface by means of adding/removing/renaming/changing a signature of a method, there is no need to change the IID
- no white space only changes (there is a lot them)

::: dom/src/jsurl/nsJSProtocolHandler.cpp
@@ +704,5 @@
>  
> +NS_IMETHODIMP
> +nsJSChannel::GetRedirects(nsIArray **aRedirects)
> +{
> +  return NS_ERROR_NOT_IMPLEMENTED;

but we can redirect from http to js I think.

::: netwerk/base/public/nsIChannel.idl
@@ +126,5 @@
>       */
>      attribute int64_t contentLength;
>  
>      /**
> +     * An array of nsIURIs associated with this channel.

the comment needs to be much more datailed

::: netwerk/base/src/nsBaseChannel.cpp
@@ +544,5 @@
>  
>  NS_IMETHODIMP
> +nsBaseChannel::GetRedirects(nsIArray **aRedirects)
> +{
> +  return NS_ERROR_NOT_IMPLEMENTED;

this definitely should be implemented

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +75,5 @@
>  HttpBaseChannel::~HttpBaseChannel()
>  {
>    LOG(("Destroying HttpBaseChannel @%x\n", this));
>  
> +  mRedirects.Clear();

no, the array should do this automatically

@@ +80,5 @@
>    // Make sure we don't leak
>    CleanRedirectCacheChainIfNecessary();
>  }
>  
> +NS_IMETHODIMP HttpBaseChannel::GetRedirects(nsIArray * *aRedirects) {

{ on a new line

@@ +1929,5 @@
> +    // Set up redirects.
> +    nsCOMPtr<nsIArray> redirects;
> +    rv = GetRedirects(getter_AddRefs(redirects));
> +    if (redirects) {
> +      httpInternal->SetRedirects(redirects);

why not to pass mRedirects here?

::: netwerk/protocol/http/HttpBaseChannel.h
@@ +313,5 @@
>    uint32_t                          mSuspendCount;
>  
>    nsCOMPtr<nsIURI>                  mAPIRedirectToURI;
>    nsAutoPtr<nsTArray<nsCString> >   mRedirectedCachekeys;
> +  nsCOMArray<nsISupports>           mRedirects;

nsTArray<nsCOMPtr<nsIURI> >

::: netwerk/protocol/http/HttpChannelChild.h
@@ +86,5 @@
>    NS_IMETHOD ResumeAt(uint64_t startPos, const nsACString& entityID);
>  
> +  NS_IMETHOD GetRedirects(nsIArray **aRedirects) {
> +    return NS_ERROR_NOT_IMPLEMENTED;
> +  }

oh, this definitely needs to be implemented

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +4134,5 @@
>      }
>  
> +    // Add this to our chain of redirects
> +    LOG(("Adding %s to redirect chain [this=%x]", mSpec.get(), this));
> +    mRedirects.AppendObject(mURI);

but this channel has not redirected at this time yet.  the redirect can be vetoed.

you need to append and carry forward the source channel uri when the new channel is actually AsyncOpen()ing.  not sooner.

Comment 28

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 8405095 [details] [diff] [review]
Pass redirects on nsIChannel where available (

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

Hi Honza,

Thanks for the feedback. I would love for this to be implemented by a necko peer. Re: unimplemented GetRedirects, this was the minimal set of changes that I needed to get the redirects into my use case (DownloadManager), and obviously I don't know the best way to get to the redirects for all of the many subclasses of nsIChannel.

Re: implementing it on nsIChannel, this was the easiest way I could think of to get something that would satisfy Jonas's future use case and my own current one -- I don't know if and when https://etherpad.mozilla.org/BetterNeckoSecurityHooks will happen. Maybe it's better not to expose it on nsIChannel, only on nsIHttpChannelInternal, in the spirit of incremental progress. The service that you suggest would also suit me fine, so long as the redirects are guaranteed to be registered by the time the final channel is reached.

What's the best way to proceed? This is blocking bug 662819 which reduces the amount of malware that Firefox users download by half, and I was hoping to have it before bug 933432.

Thanks,
Monica

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +1929,5 @@
> +    // Set up redirects.
> +    nsCOMPtr<nsIArray> redirects;
> +    rv = GetRedirects(getter_AddRefs(redirects));
> +    if (redirects) {
> +      httpInternal->SetRedirects(redirects);

nsIArray and nsCOMArray are not compatible types.

::: netwerk/protocol/http/HttpChannelChild.h
@@ +86,5 @@
>    NS_IMETHOD ResumeAt(uint64_t startPos, const nsACString& entityID);
>  
> +  NS_IMETHOD GetRedirects(nsIArray **aRedirects) {
> +    return NS_ERROR_NOT_IMPLEMENTED;
> +  }

This can just inherit from its parent HttpBaseChannel::GetRedirects.

Comment 29

[[:mmc] Monica Chew (no longer reading bugmail)]

Hi Honza,

Would you be more amenable to nsIHttpChannelInternal.getRedirects? There are only 2 subclasses of it (I think). If so, I'll work on it next week.

Thanks,
Monica

Comment 30

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

Could we expose this as a list of principals rather than a list of URIs. That way it's easier to do correct security checks using the various items in the redirect chain. I.e. it's easier to check that each server that we've gone through has permission to perform a particular action.

And getting a URI from a principal is trivial.

Comment 31

[Patrick McManus [:mcmanus]]

Comment on attachment 8405095 [details] [diff] [review]
Pass redirects on nsIChannel where available (

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

Hey Monica - I'm not sure I understand the premise of this bug. help me out :)

What I struggle with is that given that you are specifically trying to make this hook for the download manager, why don't you just use nsIChannelEventSink.asyncOnChannelRedirect() to track the chain in the download manager itself?

https://bugzilla.mozilla.org/show_bug.cgi?id=662819#c13 seems to suggest that, but I don't see it addressed as a possibility.

If we're going to implement a list as a new interface, then let's add a new one redirectedChannel.idl or somesuch. HTTP can implement it. That ought to keep the noise down to a dull roar.

I think jonas is right in comment 30 - we want to do this based on principals not URIs. I read upstream you were seeing a lot of null principals - I bet jonas can point you in the right direction to get that chased down. They are pretty much setup above necko, but necko relies on them too.

Comment 32

[[:mmc] Monica Chew (no longer reading bugmail)]

Thanks, Patrick! We chatted over irc and decided to try the nsIChannelEventSink, which I didn't understand previously.

Comment 33

[:Paolo Amadini]

(In reply to Jonas Sicking (:sicking) from comment #30)
> Could we expose this as a list of principals rather than a list of URIs.
> That way it's easier to do correct security checks using the various items
> in the redirect chain. I.e. it's easier to check that each server that we've
> gone through has permission to perform a particular action.
> 
> And getting a URI from a principal is trivial.

The purpose of this bug is not to perform security checks but rather to send the information to a remote service that will use or store it for malware tracking purposes. Thus, we need the actual detailed URI of each element in the chain.

While I'm here, just a reminder that we might need a form of sanitization of all the URIs we send to the remote service, and I'd suggest to centralize it in the Application Reputation code for all the remotely sent URIs including the referrer, redirects, and final location.

Also, I'm still not sure we send the suggested filename from "Content-Disposition" rather than what the user chose manually when saving (but this is a topic specific to downloads).

Comment 34

[[:mmc] Monica Chew (no longer reading bugmail)]

Shoot, I just saw Paolo's comment https://bugzilla.mozilla.org/show_bug.cgi?id=1004731#c1. I think this is actually required.

Comment 35

[Patrick McManus [:mcmanus]]

hey monica - as you say in comment 34, it looks like you basically do need to track the redirect chain of arbitrary channels - not just ones initiated from the download manager. so something like this makes sense and I'll take a patch for the functionality.

I think if you use my advice from comment 31 you'll be able to make good progress - limiting this to a newly minted interface (instead of extending nsIChannel) is going to greatly reduce the complexity of the patch.

I'm happy for you to author and I'll be your reviewer and give advice as needed on the netwerk bits.

Jonas and others have expressed a preference for a chain of principals instead of uris. Let's figure out if that's workable or not for you so everyone can be on the same page.

Let me know if we need something else to get unstuck.

Comment 36

[[:mmc] Monica Chew (no longer reading bugmail)]

Thanks, Patrick! I'll hopefully have time to work on this early next week.

About principals vs. uris, I'm fine with principals except that they are null where I don't expect them to be. If I can't figure that out, I will bug you.

Comment 37

[Patrick McManus [:mcmanus]]

the principal is basically set outside of necko, so we might need {jonas, gavin, boris} to help track it down.. they've helped on that in the past.

Comment 38

[[:mmc] Monica Chew (no longer reading bugmail)]

Attachment: Add nsIRedirectChannel and make nsHttpChannel implement it (

Comment 39

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 8424171 [details] [diff] [review]
Add nsIRedirectChannel and make nsHttpChannel implement it (

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

Hi Patrick,

I found the null principal bug. This needs more testing, but please review the interface and location of the hooks to see if they make sense.

Thanks,
Monica

Comment 40

[Patrick McManus [:mcmanus]]

Comment on attachment 8424171 [details] [diff] [review]
Add nsIRedirectChannel and make nsHttpChannel implement it (

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

Hi - I didn't review the exact locations of your AddRedirect calls (or the test), but the interface looks like I presumed. Much simpler than the last approach, eh?

don't forget you'll need the e10s bits for this too - to essentially copy this stuff between the parent (networking) and child processes. you can look in httpchannelparent for how some of these attributes get moved around. I'm not sure if there is an example for an array. jduell is the expert on that stuff.

Comment 41

[Jason Duell]

Talking to mmc it sounds like we don't need this info in the child, so instead of having HttpBaseChannel implement nsIRedirectionChannel we can just have nsHttpChannel do it, and skip e10s work.

Comment 42

[Honza Bambas (:mayhemer)]

Comment on attachment 8424171 [details] [diff] [review]
Add nsIRedirectChannel and make nsHttpChannel implement it (

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

::: netwerk/test/unit/test_redirect_passing.js
@@ +46,5 @@
> +  var chan = request.QueryInterface(Ci.nsIRedirectChannel);
> +  do_check_eq(1, chan.redirects.length);
> +  let redirects = chan.redirects.enumerate();
> +  let redirect = redirects.getNext().QueryInterface(Ci.nsIPrincipal);
> +  do_check_eq(initialURI, redirect.URI.spec);

have a new test please and do more then one redirect (feel free to copy this tests).  this particular test is dedicated only to testing one of the asynchronous code paths and should not be changed.

Comment 43

[Patrick McManus [:mcmanus]]

(In reply to Jason Duell (:jduell) from comment #41)
> Talking to mmc it sounds like we don't need this info in the child, so
> instead of having HttpBaseChannel implement nsIRedirectionChannel we can
> just have nsHttpChannel do it, and skip e10s work.

It seems odd to have an nsI interface that isn't accessible from js in the child.. I understand this use case doesn't require it, but don't you think it should be there?

Comment 44

[[:mmc] Monica Chew (no longer reading bugmail)]

(In reply to Jason Duell (:jduell) from comment #41)
> Talking to mmc it sounds like we don't need this info in the child, so
> instead of having HttpBaseChannel implement nsIRedirectionChannel we can
> just have nsHttpChannel do it, and skip e10s work.

I verified this just now in the eventual caller. From jduell over IRC, to make this available in HttpChannelChild you'd just pass the list of URIs as part of the PHttpChannel.OnStartRequest message (see netwerk/protocol/http/PHttpChannel.ipdl), but there was some concern about passing long lists of URIs.

The from Google is that they are OK with us sending them queries without redirect information for 1 cycle (FF 31) only. Should we start with the nsI that's only available in nsHttpChannel, then extend to HttpChannelChild if need be to avoid overhead, since there are no callers in the child process right now?

Thanks,
Monica

Comment 45

[Patrick McManus [:mcmanus]]

hey monica - we talked about whether or not this interface needs to be accessible in the child too during today's necko meeting.

The consensus was that interfaces should be available in both parent and child unless
 1] its a ton of work that just isn't needed right now (i.e. cost prohibitive)
or 2] its a ton of data that slows things down (i.e. cost prohibitive :)) on a common path.

We agreed that in this case, this interface should work in both parent and child.

however honza had a good insight - essentially, you don't need to pass this information back and forth at GetRedirects() time via ipdl because both the parent and the child should already be receiving the various redirect API mechanisms.. you could just get rid of the addredirect() interface and have the channel do that internally in both the parent and the child.

jason and/or honza will be the expert here.

Comment 46

[[:mmc] Monica Chew (no longer reading bugmail)]

Thanks, I will try to figure out Honza's advice.

Comment 47

[[:mmc] Monica Chew (no longer reading bugmail)]

Attachment: Add nsIRedirectChannel and make nsHttpChannel implement it (

Comment 48

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 8427240 [details] [diff] [review]
Add nsIRedirectChannel and make nsHttpChannel implement it (

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

Hi Patrick and Honza,

I left nsIRedirectChannel.addRedirect in, because HttpBaseChannel never constructs another HttpBaseChannel directly, so it needs access through an interface.

Honza had previously mentioned that it was incorrect to add the current URI to the redirect chain of the new channel unless the new channel was AsyncOpened. After thinking about it, I think that a full redirect chain on an unopened channel can still be useful, and I'm not sure it's worth the effort trying to prevent the last redirect from being added if the channel is never opened.

Thanks,
Monica

Comment 49

[[:mmc] Monica Chew (no longer reading bugmail)]

Attachment: Add nsIRedirectChannel and make nsHttpChannel implement it (

Comment 50

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 8427267 [details] [diff] [review]
Add nsIRedirectChannel and make nsHttpChannel implement it (

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

Missed some signed/unsigned comparisons that broke TBPL.

Comment 51

[Honza Bambas (:mayhemer)]

Comment on attachment 8427267 [details] [diff] [review]
Add nsIRedirectChannel and make nsHttpChannel implement it (

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

Much nicer!  Thanks.
f+, would check on the final patch.

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +1696,5 @@
> +NS_IMETHODIMP
> +HttpBaseChannel::GetRedirects(nsIArray * *aRedirects)
> +{
> +  nsCOMPtr<nsIMutableArray> redirects =
> +    do_CreateInstance(NS_ARRAY_CONTRACTID);

nsresult rv;

nsCOMPtr<nsIMutableArray> redirects =		do_CreateInstance(NS_ARRAY_CONTRACTID, &rv);
NS_ENSURE_SUCCESS(rv, rv);

and bellow this line now redirects will be non-null

@@ +1975,5 @@
> +    }
> +
> +    // Add our own principal to the redirect information on the new channel. If
> +    // the redirect is vetoed, then newChannel->AsyncOpen won't be called.
> +    // However, the new channel's redirect chain will still be complete.

Yes, I think it's alright

::: netwerk/protocol/http/moz.build
@@ +15,5 @@
>      'nsIHttpChannelInternal.idl',
>      'nsIHttpEventSink.idl',
>      'nsIHttpHeaderVisitor.idl',
>      'nsIHttpProtocolHandler.idl',
> +    'nsIRedirectChannel.idl',

the interface should be in netwerk/base

::: netwerk/protocol/http/nsIRedirectChannel.idl
@@ +25,5 @@
> +
> +  /**
> +   * Add a new nsIPrincipal to the redirect chain.
> +   */
> +  void addRedirect(in nsIPrincipal aPrincipal);

I'm thinking of putting this setter to nsIHttpChannelInternal interface.  This interface (nsIRedirectChannel) should only be readonly history view.

Also I'm thinking of renaming it to nsIRedirectHistory instead.

::: netwerk/test/unit/test_redirect_channel.js
@@ +5,5 @@
> +});
> +
> +var httpServer = null;
> +// Need to randomize, because apparently no one clears our cache
> +var randomPath = "/redirect/" + Math.random();

not true anymore!

@@ +7,5 @@
> +var httpServer = null;
> +// Need to randomize, because apparently no one clears our cache
> +var randomPath = "/redirect/" + Math.random();
> +var redirects = [];
> +let numRedirects = 10;

const ?

@@ +52,5 @@
> +      httpServer.registerPathHandler(randomPath, redirectHandler.bind(this, i));
> +    }
> +  }
> +  // The last one doesn't redirect
> +  httpServer.registerPathHandler(redirects[numRedirects - 1], contentHandler);

maybe put it as else to the |if (i < numRedirects - 1)| to the cycle to make it more clear what you are doing?

Comment 52

[[:mmc] Monica Chew (no longer reading bugmail)]

Attachment: Implement nsIRedirectHistory (

Comment 53

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 8429587 [details] [diff] [review]
Implement nsIRedirectHistory (

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

Thanks Honza, please take another look. Moved addRedirects to nsIHttpChannelInternal and renamed to nsIRedirectHistory.

Comment 54

[Honza Bambas (:mayhemer)]

Comment on attachment 8429587 [details] [diff] [review]
Implement nsIRedirectHistory (

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

please push to try before landing, locally tested, the new tests are passing.

::: netwerk/base/public/nsIRedirectHistory.idl
@@ +9,5 @@
> + */
> +#include "nsISupports.idl"
> +
> +interface nsIArray;
> +interface nsIPrincipal;

probably no need to declare nsIPrincipal

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +1969,5 @@
> +      mRedirects[i]->GetURI(getter_AddRefs(uri));
> +      nsCString spec;
> +      uri->GetSpec(spec);
> +      LOG(("HttpBaseChannel::SetupReplacementChannel adding redirect %s "
> +           "[this=%p]", spec.get(), this));

wrap getting the spec + LOG() in #ifdef PR_LOGGING

Comment 55

[[:mmc] Monica Chew (no longer reading bugmail)]

Try looks good, except for https://bugzilla.mozilla.org/show_bug.cgi?id=1005696 failures: https://tbpl.mozilla.org/?tree=Try&rev=93e3a4abdc88

> probably no need to declare nsIPrincipal

I get unknown interface errors without it. I'll try Try again and fix the logging wrapping before checking in.

Comment 56

[[:mmc] Monica Chew (no longer reading bugmail)]

Try: https://tbpl.mozilla.org/?tree=Try&rev=2b29d83b3b6a
Checked in: https://hg.mozilla.org/integration/mozilla-inbound/rev/8a1273ab0767

Comment 57

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/8a1273ab0767