User Agent: Mozilla/5.0 (Windows NT 6.1; rv:10.0) Gecko/20100101 Firefox/10.0 Steps to reproduce: I wrote a rough draft of a patch to provide XPCOM with a call to redirect an nsHTTPChannel to a new URL. The patch is based on the HSTS machinery code, but allows arbitrary redirects. Actual results: The API call appears to function as an effective replacement for NoScript's ChannelReplacement.js, which is what we currently use for URL redirects in HTTPS-Everywhere. Moreover, it appears to fix a URL spoofing bug due to that code (https://trac.torproject.org/projects/tor/ticket/5477). Expected results: This patch is a rough draft I wrote just to see if it would work. Instead of cargo-culting the HSTS code, the shared code should probably refactored into a new function. I'm also wondering: 1. Is nsIHTTPChannel is the right place for the API? 2. Do I need any additional refcounting magic for my nsIURI argument? 3. What about other channel types? Do any of them matter?

I think the goal here is to let add-ons change request connection targets when they intercept an HTTP request before it hits the wire. Things like HSTS and HTTPS-Everywhere (and probably others too) will benefit from this. It will also probably help out with consumers of a new content policy api (bug 694101).

Comment on attachment 634215 [details] [diff] [review] Patch to add a redirect API to nsIHTTPChannel (patch against FF13) bz: can you take a look from a SR point of view? The IDL will need a new UUID, but what do you think about this approach of adding a new function to nsIHttpChannel?

Comment on attachment 634215 [details] [diff] [review] Patch to add a redirect API to nsIHTTPChannel (patch against FF13) Needs docs and whatnot, but in general seems fine. Not clear whether ContinueAsyncRedirectChannelToHttps is the right thing to call (and it it is, it needs a new name).

Ok, so what are the next steps here? I imagine I could refactor the patch so that the HSTS code and this API share the same implementation. Perhaps I will just switch HSTS to use the API my patch provides. I imagine you have buildbots and such that will then test the API by virtue of testing HSTS, so we won't need additional tests just to exercise it (though I am curious to learn more about your testing infrastructure). I can do that refactoring, and then deploy that patch w/ a patched HTTPS-Everywhere in Tor Browser alpha series for live testing of the API, as well. Sound like a plan?

It might be a good idea to add another test to check that cross-domain rewrites are functioning correctly, marshaling the right cookies, etc.

Mike: yes, refactor so HSTS calls your new interface. I think that's a good plan. Testing: You don't have to re-test HSTS, but you should test your new interface function to see how it works. HSTS does some similar tests, you'll want to write something like those except maybe change the domain name too and see if it gets redirected properly. Related HSTS tests: http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/tests/mochitest/stricttransportsecurity/ Maybe there's a way to make simpler tests in xpcshell like these: http://mxr.mozilla.org/mozilla-central/source/netwerk/test/unit/test_redirect_loop.js In the test, listen to HTTP requests like HTTPS-everywhere does, then redirect and see if the right URLs result when you call the new API.

Ok. I'll see if I can refactor the patch sometime this week. I'm not sure about diving into mochitest right now. Realistically, that probably will be drowned out by my other responsibilities. From reading the mochitest docs, the system seems like something that should be reserved for people with expertise with it, anyways. Probably will get higher quality tests that way, too. The other thing that I guess we should also discuss is races in the dispatch between HSTS and the API call. I'm not really familiar with the async dispatch mechanism used. It looks like it might only support one outstanding redirect request on a channel at a time? That by itself might not be too bad.. But are there any other obvious potential issues?

Mike: if you have a proof of concept simpler than the whole HTTPS-Everywhere .xpi (a minimized test-case that we can run manually via add-on or web site), I can help find resources to turn it into tests for our test suite.

Well before we get to specific test discussion beyond simply leveraging existing HSTS tests, I still have some questions. I've begun refactoring my patch to explicitly share HSTS code and perhaps also rename the ContinueAsyncRedirectChannelToHttps() call. Along the way, the following questions came up: 1. Can you explain why the HSTS code needs to use the Async callback mechanism? If I'm guessing right, it's because you need to unwind the callstack from what could be a series of nsIObserver events wrt the cache and other such crazyness... Is that right? 2. Can ignoring this dispatch lead to races in the dispatch between HSTS and the API call? Should I be storing the redirect target url in a member variable, and also implement my own AsyncCall() callback? 3. I'm not sure I'm getting the XPCOM JS + nsHTTPChannel thread interactions with how the async dispatch mechanism is being used in the first place. It looks like it might only support one outstanding redirect request on a channel at a time? What ensures this? Should I worry about allowing more than one, or should we just go with "most recent one survives"? I think the latter. Agree? 4. If so, can we simplify the answer to #2 by doing nothing (as in the previously attached version of the patch)? If nsHTTPChannel expertise is otherwise occupied, we can try to answer these questions through testing, but it seems better to get answers up front?

> Can you explain why the HSTS code needs to use the Async callback mechanism? If you mean the async redirect API (OnRedirectVerifyCallback), it's most fundamentally because under e10s we need to get approval/denial of the redirect from the child process, and we don't want to block on that so it can't be synchronous. > It looks like it might only support one outstanding redirect request on a channel at a time? Yes, and we wait for all redirect callbacks before proceeding to anything that could cause another redirect, so redirects should proceed serially. cc-ing honza, who wrote the code that switched us from a sync redirect API to the async one...

Do we need to allow the new asyncRedirect() call during redirects (i.e. can a redirect observer call it to redirect a redirect?). I hope not. It might be good to explicitly note in the IDL when this call is allowed to be made (on-modify-request observers, etc), and explicitly forbid it in other cases (and enforce that in the code), else this could become a real complexity sinkhole.

The two places we will call the API are from a nsIChannelEventSink.asyncOnChannelRedirect() observer (to decide if we want to redirect the destination URL of the newChannel destination) which we make synchronous by executing the callback immediately, as well as inside an "http-on-modify-request" observer. So, yes, we do need to potentially redirect a redirect in some cases. I don't believe I tested that case in my patch. Perhaps it will explode? As for the async stuff, what I'm talking about is the AsyncCall() usage by HSTS and several other things around the nsHTTPChannel code. The problem I am facing for refactoring is that I need to keep a destination URL stashed somewhere if I am to use this AsyncCall() machinery for the API (since AsyncCall does not support scheduling callbacks that take arguments). HSTS doesn't have to do this because it merely changes the URL scheme in place, so it can use AsyncCall as-is with no arguments or cached member state across the callback.. Does that help explain my concerns? I also want to understand the related nsHttpChannel::mCallonResume mechanics in general, too. Is all of that for e10s, too? (And is e10s active anywhere other than FF mobile? I thought it was dying/dead). Dropping some URLs on me is fine for the latter part (if any good ones exist, I couldn't find them). I just want to understand the threading model for this stuff, as I suspect this won't be the last semi-complicated XPCOM API I end up writing.

I'd make the implementation a different way: - let asyncRedirect be allowed to call only before asyncOpen exits (on an unopenned channel) - let it just set the target url to force redirect to (in a new member, that is then non-null) - change asyncOpen to check on this new member before it moves to load of the current URL (either from cache or net) and do what actually the implementation of asyncRedirect in the current patch does Then asyncRedirect can be called at any time before a channel is open.

According the mCallOnResume: we prevent operations on a channel when it is suspended. So, when the channel is suspended, we just save the operation demanded in the mCallOnResume pointer. When the channel is resumed again, we call this operation. So far, there was no need to stack the functions to be called, there was always only one. It is not anything related to e10s, it is just and only for suspend/resume logic. In your patch, btw, it is wrong. You are storing wrong function to be called on resume. With my previous suggestion it will be simpler to implement on-resume logic.

bz: nit: why not just call it 'redirect', instead of 'asyncRedirect'? More and more of our functions are async, so I think the prefix becomes noise, and we don't have a synchronous version to disambiguate from. > I thought e10s was dying/dead e10s is being used for B2G, so it's very much alive. > let asyncRedirect be [callable] only before asyncOpen exits... So check for some "mRedirectedTo" URI being non-null at http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannel.cpp#4433 and do a redirect instead of opening normally, right? This handles the 'on-modify-request' observer case. And I think we could piggyback on it to support the case of trying to redirect during asyncOnChannelRedirect(). (That said, if you can get away w/o supporting that case, good.) If we do need to support it, I think it may be as simple as setting mRedirectedTo on the *new* httpchannel (i.e. redirect observers would have to call asyncRedirect on the new channel, not the old, which should probably be an error if called after AsyncOpen), and we'll wind up checking that flag normally during AsyncOpen and do the redirect "as-if" it had been set by a on-modify-request observer, so we'll reissue all the redirect logic again, to the new URI. Maybe Honza was already implying that. He's smarter than I am :) To make e10s work with this you'll need to send mRedirectedTo from the child to the parent as part of HttpChannelChild::SendAsyncOpen() so we capture asyncRedirect() calls that were made on the child. Add a public but not XPCOM/IDL SetRedirectedToInternal() function to nsHttpChannel that lets HttpChannelParent pass the mRedirectedTo value down to the nsHttpChannel (see section in nsHttpChannel.h that says "for internal necko use only") Have we thought through what happens if the redirect fails (ex: is cancelled by a different observer, has a bogus URI, etc)? For HTTP redirects we fall back to displaying the body of the 30x reply. But you don't provide one with asyncRedirect, so we probably want to make sure we see a boilerplate error page instead. re: AsyncCall usage: the point of AsyncCall is to call something that you can't call within the current event dispatch. The classic example for nsHttpChannel is if we need to cancel in AsyncOpen after we've called on-modify-request observers: we're committed to calling the listener's OnStart/StopRequest methods, but we can't call them while the AsyncOpen function is in progress (else a JS program could see it's OnStartRequest callback being called *while* it hasn't returned from AsyncOpen), so we schedule an event that will call them after we return. I think there are some places in nsHttpChannel where we may no longer need to use AsyncCall, but you'd need to make sure before you remove any of them.

(In reply to Jason Duell (:jduell) from comment #15) > > let asyncRedirect be [callable] only before asyncOpen exits... > > So check for some "mRedirectedTo" URI being non-null at We could use mRedirectURL instead. However, that would become two-purpose member. Need to check it is a good idea. In any case, there will be some bit of confusion anyway. > > > http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/ > nsHttpChannel.cpp#4433 > > and do a redirect instead of opening normally, right? > > This handles the 'on-modify-request' observer case. And I think we could > piggyback on it to support the case of trying to redirect during > asyncOnChannelRedirect(). (That said, if you can get away w/o supporting > that case, good.) If we do need to support it, I think it may be as simple > as setting mRedirectedTo on the *new* httpchannel (i.e. redirect observers > would have to call asyncRedirect on the new channel, not the old, which > should probably be an error if called after AsyncOpen), and we'll wind up > checking that flag normally during AsyncOpen and do the redirect "as-if" it > had been set by a on-modify-request observer, so we'll reissue all the > redirect logic again, to the new URI. > > Maybe Honza was already implying that. He's smarter than I am :) Exactly. That's it! Question about the "asyncRedirect" method behavior is what has to happen when called more then ones with a non-null URL. Override or fail? And what about passing null URL to cancel the redirect later? > > To make e10s work with this you'll need to send mRedirectedTo from the child > to the parent as part of HttpChannelChild::SendAsyncOpen() so we capture > asyncRedirect() calls that were made on the child. Add a public but not > XPCOM/IDL SetRedirectedToInternal() function to nsHttpChannel that lets > HttpChannelParent pass the mRedirectedTo value down to the nsHttpChannel > (see section in nsHttpChannel.h that says "for internal necko use only") I assume the latest moment on the child (as well as chrome, btw) to call "asyncRedirect" is on-modify-request, that is called synchronously before we call SendAsyncOpen, so we already have that info in hands to send it thought. So, http channel parent can just call the IDL method, IMO. > > Have we thought through what happens if the redirect fails (ex: is cancelled > by a different observer, has a bogus URI, etc)? For HTTP redirects we fall > back to displaying the body of the 30x reply. But you don't provide one > with asyncRedirect, so we probably want to make sure we see a boilerplate > error page instead. I think there is the original URI of the channel it has been opened with, no? Redirect, when *vetoed* or the new channel asyncOpen fails, will not proceed and the original URI will load instead.

> And what about passing null URL to cancel the redirect later? I guess that's OK, provided it's called before asyncOpen completes. > what has to happen when called more then once For now, as gross as it is, I'd suggest we just allow the last caller on redirect() to win (as long as they're calling before on-modify-request completes). Either that or we can throw an exception. > So, http channel parent can just call the IDL method, IMO. Ah, you're right. >> what happens if the redirect fails > > the original URI will load instead That's fine as long as we're not using redirect() for security purposes (i.e. to make sure the original URI never renders). I'm not clear on why we're adding this API, but comment 0 sounds like it might involve replacing the HTTPS-Everywhere infrastructure--are we OK falling back to HTTP?

The backstory about why this is happening: https://trac.torproject.org/projects/tor/ticket/5477#comment:37 bugs like that are fundamentally consequences of this: https://bugzilla.mozilla.org/show_bug.cgi?id=692915 As for letting the last redirect win... I think that would be okay. Especially if we will get another callback via either on-modify-request or nsIChannelEventSink/onChannelRedirect to try and redirect again (if there is a recursive redirect loop with a site or another extension, our code will notice and back off)

And I'm guessing simply falling back to the original channel if the redirect fails is OK, in that if the redirect caller can observe OnRedirectResult, they can cancel that channel if they don't want the user to see the original channel's content. (Note that with the current nsIRedirectResultListener this requires being the channel's callbacks object, which is perhaps not ideal: if you need that changed, it's possible (AFAIK only internal code uses that interface right now).

(In reply to Jason Duell (:jduell) from comment #17) > That's fine as long as we're not using redirect() for security purposes > (i.e. to make sure the original URI never renders). I'm not clear on why > we're adding this API, but comment 0 sounds like it might involve replacing > the HTTPS-Everywhere infrastructure--are we OK falling back to HTTP? Good point. Probably not. (In reply to Jason Duell (:jduell) from comment #19) > And I'm guessing simply falling back to the original channel if the redirect > fails is OK, in that if the redirect caller can observe OnRedirectResult, > they can cancel that channel if they don't want the user to see the original > channel's content. (Note that with the current nsIRedirectResultListener > this requires being the channel's callbacks object, which is perhaps not > ideal: if you need that changed, it's possible (AFAIK only internal code > uses that interface right now). If we turn OnRedirectResult callback to be global then we just duplicate the AsyncOnChannelRedirect API. Anyone in OnRedirectResult can cancel any of the two channels (old or new one). We can again get to some unexpected state. However, we can make changes on the channels during invocation of that callback forbidden with some flag. But that sounds a bit crazy...

(In reply to Honza Bambas (:mayhemer) from comment #20) > > I'm not clear on why > > we're adding this API, but comment 0 sounds like it might involve replacing > > the HTTPS-Everywhere infrastructure--are we OK falling back to HTTP? > > Good point. Probably not. > For HTTPS Everywhere, we don't cancel requests if they're being redirected back to HTTP; we prefer insecurity to outright breakage. But for HSTS, which also uses this mechanism, the correct response is breakage (cancelling the request) I think.

To help test the various loop and redirect-fight cases, I set up a server that sends the HSTS header, but has some URLs that redirect back to HTTP. Try visiting these in order: 1. http://theobroma.info/infinite-loop.html (observe HTTP response) 2. https://theobroma.info/ (sets HSTS successfully) 3. https://theobroma.info/infinite-loop.html (infinite loop 1) 4. http://theobroma.info/infinite-loop.html (infinite loop 2) The behaviour of 4 in FF 10esr looks like an existing Firefox bug that we should fix as we close this ticket (the loop is not detected, and FF keeps sending the request via HTTPS and being 301'd back to HTTP)

Sets a member to be used during AsyncOpen. Also cleans up some function naming.

I tested that patch against the 4 urls above, and the behavior appears to be the same. I inspected the code a bit, and I think the reason that #4 causes an infinite loop still is because the error message from #3 is only displayed for non-cached responses... So somewhere in the cached response codepath there should be a check for the redirect limit, I guess? Not sure where that needs to go.

> If we turn OnRedirectResult callback to be global then we just duplicate > the AsyncOnChannelRedirect API. Doesn't seem that way to me. You can't veto/approve the redirect--that's already decided--but you get to see the result, safely update your mChannel, and cancel the channel if you don't like it (seems like that would be rare, as you'd have to have approved it beforehand). Unfortunately, the nsIRedirectResultListener IDL doesn't give you the channel that you're redirecting to, so it seems the only safe way to update would be to save both original/redirectedTo channels during AsyncOnChannelRedirect, then set mChannel to redirectedTo if redirect succeeded. It might make things easier if OnRedirectResult also passes whatever channel is being used now, to make updating mChannel trivial.

jduell and mayhemer: instead of worrying about a complicated veto path, I opted for "most recent API user prior to AsyncOpen() wins". As for redirecting redirects, I'm assuming that using the new nsIHTTPChannel.redirectTo API on the newchannel parameter of nsIChannelEventSync.AsyncOnChannelRedirect is valid. I believe those two properties make the API sufficient for our use in HTTPS-Everywhere. However, I have no idea where to even begin tweaking the API to work for e10s. Someone else might have to do that bit, but with very detailed instruction I suppose I could give it a try... Unless it will just magically work already, given simple assumptions about vetoing...

e10s: it won't just work, sorry :) You have to carry the redirect url as an arg in the e10s constructor. Check [1] [2] [3]. [1] http://hg.mozilla.org/mozilla-central/annotate/ba8463beab13/netwerk/protocol/http/HttpChannelChild.cpp#l1037 [2] http://hg.mozilla.org/mozilla-central/annotate/ba8463beab13/netwerk/protocol/http/HttpChannelParent.cpp#l95 [3] http://hg.mozilla.org/mozilla-central/annotate/ba8463beab13/netwerk/protocol/http/PHttpChannel.ipdl#l31 Rebuild _obj/ipc/ipdl to take changes to ipdl in affect.

Mike, your policies in comment 26 sound fine to me. Remaining policy issue is what to display if redirect is vetoed: I think my answer would be to fallback to our usual default (render the initial channel), and it's up to observers to Cancel in OnRedirectResult(false) if they don't want the initial channel to be rendered.

I think I am fine making HTTPS-Everywhere fail channels rather than fall back to http on error. It seems that failure will be due to a rule bug, and it is perhaps best that those be obvious to us immediately? It also looks to me that if we share the HSTS code, the channel will be shut down in ContinueAsyncRedirectChannelToURI(), though I am unable to find the explicit Cancel call there. There is only an mStatus update to the failure code and a call to remove the channel from the loadgroup... However, there is also a codepath that doesn't lead to this error condition update: https://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannel.cpp#1652 Should we worry about that path? Does it need an explicit Cancel() there? I will also see about following mayhemer's urls + your guys e10s comments to support it... It sounds like it should be straight-forward enough, and it's a bit clearer to me now (though I don't fully understand parent vs child vs main channel separation and interaction yet, but perhaps it will soon become clear).

> I am unable to find the explicit Cancel call there We seem to treat setting mStatus to a failure code as equivalent to Canceling in nsHttpchannel--I've never been 100% clear that it's the exact same in all cases. Note that I'm going to land bug 773475 soon (after the next B2G milestone) and that will change (improve!) the logic a little. You may want to apply it already in your working code...

(In reply to Mike Perry from comment #29) > I think I am fine making HTTPS-Everywhere fail channels rather than fall > back to http on error. It seems that failure will be due to a rule bug, and > it is perhaps best that those be obvious to us immediately? > > It also looks to me that if we share the HSTS code, the channel will be shut > down in ContinueAsyncRedirectChannelToURI(), though I am unable to find the > explicit Cancel call there. There is only an mStatus update to the failure > code and a call to remove the channel from the loadgroup... However, there > is also a codepath that doesn't lead to this error condition update: > https://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/ > nsHttpChannel.cpp#1652 In that code, AutoRedirectVetoNotifier notifier(this); line is the important one. It calls OnRedirectResult(false) where you can cancel the initial channel.

Yes, and my change in bug 773475 will make the AsyncOpen in ContinueAsyncRedirectChannelToHttps no longer fail so we won't trigger the AutoRedirectVetoNotifier. Which is good, because whatever cancelled the redirected-to channel didn't ask to veto it: there's a big difference!

Ok, I have implemented e10s. It is untested. I also added a direct call to Cancel() the initial channel in the HSTS failure codepath I mentioned in comment 29. I opted for the direct call because it seemed simpler than going back and forth between the nsIRedirectResultlistener through the veto path.

Mike: the easiest way to test e10s (and non-e10s for that matter) is to write an xpcshell test that uses the new redirect call. To make an e10s version, just add a "*_wrap.js" file in netwerk/test/unit_ipc that calls the 'real' test in netwerk/test/unit. See the code in there for examples.

Regarding the test I just attached: I haven't been able to run it, unfortunately, because I don't have a working mozilla build environment. Mike ran an earlier variant in his Tor Browser build instance; it seemed to run but didn't seem to behave differently when the tests are rigged to fail (!). So we probably need a little bit of assistance from someone with more xpcshell experience to get this over the line.

Here is a first draft of a test. There is a successful test of within-domain and cross-domain redirects using XMLHttpRequest. However my attempt to test the API's treatment of a channel.asyncOpen() request seemed to trigger a failure in head_channels.js's test implementation of ChannelListener ("request reports itself as not pending from onDataAvailable!"). Jason, do you know whether this is indicative of a real problem, a bug in my test, or a false assumption in head_channel.js?

Looks like bug 748117? Try setting Content-type on the response.

Okay, this test now works XMLHttpRequest and asyncOpen() calls. I think open() still gets an NS_BINDING_REDIRECTED error (which may be the correct behaviour, I'm not sure). Remaining todos: figure out what if anything to test about channel.open(), wrap for e10s.

hmm, we hardly even do a synchronous open() on an HTTP channel, but it looks like it should be handled correctly (see HttpBaseChannel::Open. I would have expected the listener to get transferred to the new channel rather than staying with the old one (which returns NS_BINDING_REDIRECTED). I'm trying to ping biesi on IRC to see if he has an opinion. Meanwhile it would be interesting to write a different xpcshell test that does open() with a regular redirect (ie one trigger via a 30X code and Location: header) and see how it behaves. If it works, then we should make your case work. If it also returns BINDING_REDIRECTED we can probably file a followup (or maybe that it correct, though it doesn't seem like it to me).

Here are tests for Mike's patch. I've covered e10s, but left channel.open() untested for the moment. It seems that the behaviour it currently exhibits (erroring with NS_BINDING_REDIRECTED) is probably not ideal because there are probably extension developers out there using channel.open() and expecting it to work. If making channel.open() work is too much work, we should at minimum edit the nsIChannel docs to note that extensions should not call .open() to make HTTP requests. This patch is against mozilla-central, so the xpcshell.ini changes might require manual merging.

(In reply to Jason Duell (:jduell) from comment #40) > Meanwhile it > would be interesting to write a different xpcshell test that does open() > with a regular redirect (ie one trigger via a 30X code and Location: header) > and see how it behaves. If it works, then we should make your case work. > If it also returns BINDING_REDIRECTED we can probably file a followup (or > maybe that it correct, though it doesn't seem like it to me). Here is that test case. It also fails with NS_BINDING_REDIRECTED, so we could kick the channel.open() issue to another bug. Unfortunately, I also noticed that in the server-side 302 redirection case, channel.responseStatus is 302, but after a redirect from the new API, trying to examine channel.responseStatus raises NS_ERROR_NOT_AVAILABLE.

(In reply to Peter Eckersley from comment #42) > Here is that test case. It also fails with NS_BINDING_REDIRECTED, so we > could kick the channel.open() issue to another bug. Unfortunately, I also > noticed that in the server-side 302 redirection case, channel.responseStatus > is 302, but after a redirect from the new API, trying to examine > channel.responseStatus raises NS_ERROR_NOT_AVAILABLE. I guess I posted that rather late last night. Of course there is no well-defined status code after a browser-internal redirect. I think we're ready to apply the two patches attached to this bug. Can that happen directly, or do we need further rounds of review?

Flagging jduell for review since he's already looked at the patches.

Comment on attachment 645172 [details] [diff] [review] Redirect API with e10s support Review of attachment 645172 [details] [diff] [review]: ----------------------------------------------------------------- It'll be a little while before I can look at this (clearing out some kilimanjaro stuff). I imagine you're just as busy, Honza, but feel free to grab if you get time before I do.

I know B2G is a giant distraction, but it would be enormously appreciated if someone could take a bit of time to review the final version of this patch. We already had superreview of an inferior earlier patch, and we need this hook to deal with extension compatibility and security issues that are probably impacting hundreds of thousands of our users.

Okay, I have a very nice bottle of Scotch to go to whoever can spare the extra effort to review this after hours. Or if Scotch isn't your thing, I may take requests ;)

I think I can take a look at the patches this or next week. If I actually start, I'll drop a note. Quickly looking at the patch: please add more context (8 lines) to your patches.

This is the Redirect API rebased to FF15.0.1 and with 10 lines of unified context. Not replacing the old patch as the additional context is probably more likely to hit conflicts.

Ok, I added the additional context and attached a second version of the patch. I left the original because I expect the additional context to cause conflicts later on. What more can we do to ensure this gets reviewed again in time for FF17 and the associated FF17 ESR release branch? If we burn another 2 months on waiting for more review, we're sure to miss the ESR deadline, right? This would mean exposing Debian, RHEL, and other ESR-series users of HTTPS-Everywhere to the original url spoofing vulnerability for another full year and a half... That seems bad...

Comment on attachment 647078 [details] Test for channel.open() with a serverside redirect Aside: marking the channel.open() patch obsolete because it is distracting and needs to move to a separate bug. I edited the docs at https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsIChannel?redirectlocale=en-US&redirectslug=nsIChannel#open() to warn extension authors not to call nsIChannel.open().

Ping! Last week I think jduell said that jdm might work on the review, and then Honza said that he would work on it. Either would be great! :)

Peter, what is the deadline for you? I think I can do the reviews tomorrow, but this week I'm a bit in a pressure, so next week would be better for me.

Also, could you please add 8 lines of context to your patches? It makes reviewing simpler. Thanks.

Comment on attachment 645172 [details] [diff] [review] Redirect API with e10s support Review of attachment 645172 [details] [diff] [review]: ----------------------------------------------------------------- Overall looks good, well done. But please read the comments. r- ::: netwerk/protocol/http/nsHttpChannel.cpp @@ +1505,5 @@ > + > + // If we've failed so far, cancel the current channel, too, > + // as both HSTS and the redirectTo codepaths prefer > + // request failure to insecurity. > + Cancel(rv); I'm against this change. Please explain why you would so much need this in core. You can always cancel the channel your self in your veto callback and also failure of this method will propagate further and will correctly set the failure state to mStatus that, when in a failure state, effectively turns the channel to be canceled. So, call to Cancel(NS_ERROR_<SOMETHING>) is redundant and may just cause unexpected results. r- for this. ::: netwerk/protocol/http/nsHttpChannel.h @@ +154,5 @@ > void SetUploadStreamHasHeaders(bool hasHeaders) > { mUploadStreamHasHeaders = hasHeaders; } > > + void SetInternalRedirectURI(nsIURI *redirectTo) > + { mInternalRedirectURI = redirectTo; } I presume this is here just to be called from the parent channel. If so, why don't you just use the RedirectTo method instead? ::: netwerk/protocol/viewsource/nsViewSourceChannel.cpp @@ +712,5 @@ > +NS_IMETHODIMP > +nsViewSourceChannel::RedirectTo(nsIURI *uri) > +{ > + return NS_ERROR_NOT_IMPLEMENTED; > +} Something tells me we have to delegate to mHttpChannel here.

Comment on attachment 647071 [details] [diff] [review] Tests for the redirect API Review of attachment 647071 [details] [diff] [review]: ----------------------------------------------------------------- Not bad, just please add a simple list of steps the test is doing. I'll check the next version quickly. Please remove trailing white spaces. ::: netwerk/test/unit/test_redirect_from_script.js @@ +85,5 @@ > + if (target) { > + var tURI = ioservice.newURI(target, null, null); > + dump("redirecting request to " + redirectedURI + "\n"); > + try { channel.redirectTo(tURI); } > + catch (e) { dump("Exception in redirectTo " + e + "\n"); } I think you should throw here (do_throw() is what you want I think). @@ +111,5 @@ > + dump("Succeeded: " + hc.requestSucceeded + "\n"); > + dump("Status code: " + hc.responseStatus + "\n"); > + dump("Received a response from the server, test header is " + hc.getResponseHeader(testHeaderName)); > + do_check_eq(hc.getResponseHeader(testHeaderName), testHeaderVal); > +}*/ Please remove this commented code. @@ +138,5 @@ > + > + var hc = req.QueryInterface(Ci.nsIHttpChannel); > + do_check_eq(hc.getResponseHeader(testHeaderName), testHeaderVal2); > + do_check_eq(buffer, redirectedText); > + done(); Hmm.. interesting you are assuming this one will be called later.. I believe you may get to situation callback2 is called sooner and the test prematurely ends. Do the test serially and not in parallel. @@ +171,5 @@ > + httpServer.registerPathHandler(redirectedPath, redirectedHandler); > + httpServer.start(4444); > + httpServer2 = new nsHttpServer(); > + httpServer2.registerPathHandler(redirectedPath, redirected2Handler); > + httpServer2.start(4445); Why do you so much need a second port? I think the different path is enough, but up to you, the work is already done. @@ +178,5 @@ > + > + testViaXHR(); // works, we pass! > + > + do_test_pending(); > + testViaAsyncOpen(); // will call done() asynchronously for cleanup put do_test_pending(); as the last line please.

This is a merged patch to the current m-c. You can use it as a base for your next version update.

(In reply to Honza Bambas (:mayhemer) from comment #56) > Why do you so much need a second port? I think the different path is > enough, but up to you, the work is already done. > I had in my head the idea that sometimes HTTP connections might be reused for a within-server redirect, but should never be across servers, and I wanted to test that the substrate realised it needed a new HTTP connection when redirectTo() changed the URI's host. I am too ignorant about necko to know whether I succeeded in achieving that (the second test would need to reuse the connection from the first), but it seemed harmless to test both the within- and across-server cases. > @@ +178,5 @@ > > + > > + testViaXHR(); // works, we pass! > > + > > + do_test_pending(); > > + testViaAsyncOpen(); // will call done() asynchronously for cleanup > > put do_test_pending(); as the last line please. Wouldn't that create the risk that do_test_finished() would be called before do_test_pending() ?

(In reply to Peter Eckersley from comment #58) > (In reply to Honza Bambas (:mayhemer) from comment #56) > > > Why do you so much need a second port? I think the different path is > > enough, but up to you, the work is already done. > > > > I had in my head the idea that sometimes HTTP connections might be reused > for a within-server redirect, but should never be across servers, and I > wanted to test that the substrate realised it needed a new HTTP connection > when redirectTo() changed the URI's host. I am too ignorant about necko to > know whether I succeeded in achieving that (the second test would need to > reuse the connection from the first), but it seemed harmless to test both > the within- and across-server cases. I don't think you need it, I don't see a purpose to do this. You are testing something out of scope of this bug. Necko may share connections to a single server (origin) when using HTTP. I don't see a reason why that should be considered any problem for redirects or privacy. Necko separates connections to pools where each pool belongs to a distinguished origin. Polls can be separated even more when using e.g. anon connections. > > > @@ +178,5 @@ > > > + > > > + testViaXHR(); // works, we pass! > > > + > > > + do_test_pending(); > > > + testViaAsyncOpen(); // will call done() asynchronously for cleanup > > > > put do_test_pending(); as the last line please. > > Wouldn't that create the risk that do_test_finished() would be called before > do_test_pending() ? No. During JS function execution, file of an event to the main thread is prohibited. I want this change since when testViaAsyncOpen() throws from any reason, we get hanging test since it has already been set as pending. It makes analyzes of the failure more complicated and kills the test execution chain.

(In reply to Honza Bambas (:mayhemer) from comment #55) > ::: netwerk/protocol/http/nsHttpChannel.cpp > @@ +1505,5 @@ > > + > > + // If we've failed so far, cancel the current channel, too, > > + // as both HSTS and the redirectTo codepaths prefer > > + // request failure to insecurity. > > + Cancel(rv); > > I'm against this change. > > Please explain why you would so much need this in core. You can always > cancel the channel your self in your veto callback and also failure of this > method will propagate further and will correctly set the failure state to > mStatus that, when in a failure state, effectively turns the channel to be > canceled. So, call to Cancel(NS_ERROR_<SOMETHING>) is redundant and may > just cause unexpected results. > > r- for this. If I understand you correctly, this would require HTTPS-Everywhere implementing an nsIRedirectResultListener, which involves replacing the channel notificationCallbacks for the channel with our own pass-through JS wrapper. This is a little hairy... Also more importantly, it doesn't help the HSTS case, for which failure also should result in the original request being canceled, too. The original HSTS code attempted to cancel the original channel in other codepaths if the redirect failed, so I was also trying to maintain that (see comment #29). If there is some byzantine path through the code that *does* result in the original channel being canceled in this failure case for both HSTS and us, I'm fine simply removing the Cancel. But I couldn't find it... If there's also some other deep magic that means that for this particular failure path, HSTS *does* want the original http URL to always load, we might be OK with that for HTTPS-Everywhere too, but I don't understand that reasoning either right now.. > ::: netwerk/protocol/http/nsHttpChannel.h > @@ +154,5 @@ > > void SetUploadStreamHasHeaders(bool hasHeaders) > > { mUploadStreamHasHeaders = hasHeaders; } > > > > + void SetInternalRedirectURI(nsIURI *redirectTo) > > + { mInternalRedirectURI = redirectTo; } > > I presume this is here just to be called from the parent channel. If so, > why don't you just use the RedirectTo method instead? Ok. > ::: netwerk/protocol/viewsource/nsViewSourceChannel.cpp > @@ +712,5 @@ > > +NS_IMETHODIMP > > +nsViewSourceChannel::RedirectTo(nsIURI *uri) > > +{ > > + return NS_ERROR_NOT_IMPLEMENTED; > > +} > > Something tells me we have to delegate to mHttpChannel here. Oh, duh. I didn't see the other methods were just calling their mHttpChannel if it was non-null. I will fix these two later issues in a new patch if you can give me some guidance on the HSTS veto/Cancel issue above.

Comment on attachment 665640 [details] [diff] [review] Redirect API with e10s support: merged to m-c Review of attachment 665640 [details] [diff] [review]: ----------------------------------------------------------------- It seems wrong to me that we're putting this logic in nsHttpChannel. What about non-HTTPS channels, such as the ones created with the raw socket API? The idea behind HTTPS Everywhere (use encrypted/authenticated channels whenever possible) apply to all protocols. We already have nsIContentPolicy. Imagine if we changed the signature of nsIContentPolicy::shouldLoad to add a new parameter: out nsIURI redirectTo, which allowed the content policy to say "Yes, load the resource, but use a different URI instead." This would provide explicit feedback to the initiator of the content load that the URI (may have) changed; this may be crucial for proper (secure) function of the initiator of the content load. In some cases, the initiator of the load may even want to overrule the addons' suggestion of a new URI (AUS and Firefox Sync come to mind here). Also, I remember Brandon Stern saying that we purposefully limit the places in which we allow nsIContentPolicy to be effective. It seems to me that set of cases where we allow (and disallow) nsIContentPolicy to be effective for a particular load should exactly match the set of cases where we allow (and disallow) these kinds of redirects. nsIContentPolicy also provides important contextual information that is essential for some HTTPS-Everywhere-like things to work. For example, we (Mozilla) would like to try to automatically "fix" some mixed-content pages by rewriting the URIs for the non-SSL resources to be SSL URIs. I am concerned about adding new kinds of internal redirects, because some code might be written with the assumption that no redirects are happening (because it uses HTTPS and it knows the server will never send a 3xx reply or do anything else that would trigger an internal redirect). Bug 798430 is one possible example of this. ::: netwerk/protocol/viewsource/nsViewSourceChannel.cpp @@ +675,5 @@ > +} > + > +// XXX: Is this the right thing to do here? Or should we have > +// made an nsIHTTPChannelRedirect that only nsHttpChannel implements? > +// Also, will this mean that some ViewSource requests may be non-https? This comment seems to make sense only for HTTPS Everywhere.

Here is the revised test API with the changes Honza asked for in comment 56: - Document the steps in the test - replace dump() erroring with do_throw() - remove commented-out hypotheticals - serialise testViaAsynOpen() to avoid a race condition - call do_test_pending() right at the end

(In reply to Brian Smith (:bsmith) from comment #61) > We already have nsIContentPolicy. Imagine if we changed the signature of > nsIContentPolicy::shouldLoad to add a new parameter: out nsIURI redirectTo, > which allowed the content policy to say "Yes, load the resource, but use a > different URI instead." This would provide explicit feedback to the > initiator of the content load that the URI (may have) changed; this may be > crucial for proper (secure) function of the initiator of the content load. > In some cases, the initiator of the load may even want to overrule the > addons' suggestion of a new URI (AUS and Firefox Sync come to mind here). Yep, that sounds good to me. However, when there is some redirectTo provided, we still need some API on channels that support redirects (http) to do the actual redirect with all the redirect-veto dance.

(In reply to Honza Bambas (:mayhemer) from comment #63) > (In reply to Brian Smith (:bsmith) from comment #61) > > We already have nsIContentPolicy. Imagine if we changed the signature of > > nsIContentPolicy::shouldLoad to add a new parameter: out nsIURI redirectTo, > > which allowed the content policy to say "Yes, load the resource, but use a > > different URI instead." This would provide explicit feedback to the > > initiator of the content load that the URI (may have) changed; this may be > > crucial for proper (secure) function of the initiator of the content load. > > In some cases, the initiator of the load may even want to overrule the > > addons' suggestion of a new URI (AUS and Firefox Sync come to mind here). > > Yep, that sounds good to me. However, when there is some redirectTo > provided, we still need some API on channels that support redirects (http) > to do the actual redirect with all the redirect-veto dance. The other problem with nsIContentPolicy is that it doesn't apply to all types of http element loads *from content*, which makes it unacceptable from a security perspective. We can't protect our users from Firesheep if all that Firesheep has to do is trigger and listen for http favicon loads, for example. Incidentally, the initial STS implementation was done via nsIContentPolicy, and eventually abandoned because of these types of leaks. On the other hand, channel-observer approaches such as our current NoScript-based channel replacement code and this API are certain to catch all http loads, allowing us to actually provide real security for our users. Related: one of the primary advantages of Firefox's XPCOM over addon platforms like Google Chrome is the is the agility XPCOM provides to addon developers to alter many aspects of browser behavior easily. I strongly recommend that Mozilla not take steps to neuter this capability. P.S. I'm still wondering about my questions about the HSTS codepaths in comment #60 wrt the Cancel(). If there's a good reason HSTS doesn't want to cancel the http load for certain types of failed https redirects, I can just omit the Cancel and post fixes for the other two items you mentioned in the review.

(In reply to Mike Perry from comment #64) > P.S. I'm still wondering about my questions about the HSTS codepaths in > comment #60 wrt the Cancel(). If there's a good reason HSTS doesn't want to > cancel the http load for certain types of failed https redirects, I can just > omit the Cancel and post fixes for the other two items you mentioned in the > review. I think I understand, but... First, the change is wrong, because when you get the failure asynchronously, you won't cancel the channel. And, just check code at the current implementation of nsHttpChannel::ContinueAsyncRedirectChannelToHttps (turned to ContinueInternalRedirectChannelToURI). We already cancel the channel on redirect failure, there is even a direct comment about it. So you don't need additional call to Cancel on the old channel! :)

(In reply to Honza Bambas (:mayhemer) from comment #65) > (In reply to Mike Perry from comment #64) > > P.S. I'm still wondering about my questions about the HSTS codepaths in > > comment #60 wrt the Cancel(). If there's a good reason HSTS doesn't want to > > cancel the http load for certain types of failed https redirects, I can just > > omit the Cancel and post fixes for the other two items you mentioned in the > > review. > > I think I understand, but... > > First, the change is wrong, because when you get the failure asynchronously, > you won't cancel the channel. > > And, just check code at the current implementation of > nsHttpChannel::ContinueAsyncRedirectChannelToHttps (turned to > ContinueInternalRedirectChannelToURI). We already cancel the channel on > redirect failure, there is even a direct comment about it. So you don't > need additional call to Cancel on the old channel! :) Oh man. Yeah, I did not see that in both cases, ContinueInternalRedirectChannelToURI eventually ends up getting called directly by callers of InternalRedirectChannelToURI() when failure is returned. I've removed the explicit Cancel, and commented the PopRedirectAsyncFunc that removes InternalRedirectChannelToURI (which was what originally caused me to worry). I also fixed the other two review issues from comment #55 in the patch I just attached.

Honza, let us know if you need anything else or whether the API and test patches are ready for a final review/merge.

(In reply to Peter Eckersley from comment #68) > Honza, let us know if you need anything else or whether the API and test > patches are ready for a final review/merge. I think you had to ask for review (or feedback) a long ago :)

Sorry, that was my lack of experience with bugzilla (and lack of permissions to flag others' patches for review) at work.

ping

Comment on attachment 670146 [details] [diff] [review] Revised test for the redirect API Review of attachment 670146 [details] [diff] [review]: ----------------------------------------------------------------- Those are just few of all the things you need to fix. I was not able to run this test, please fix it first and then resubmit. Check it with: _obj-dir$ SOLO_FILE="test_redirect_from_script.js" make -C netwerk/test/ check-one ::: netwerk/test/unit/test_redirect_from_script.js @@ +1,1 @@ > +do_load_httpd_js(); const Cc = Components.classes; const Ci = Components.interfaces; const Cu = Components.utils; const Cr = Components.results; Cu.import("resource://testing-common/httpd.js"); @@ +16,5 @@ > + * Both of the above functions tests two requests, a simpler one that > + * redirects within a server, and second that redirects to a second webserver. > + * The successful redirect is confirmed by the presence of a custom response > + * header. > + * white space @@ +19,5 @@ > + * header. > + * > + */ > + > +var httpServer = null; and httpServer2 ? @@ +42,5 @@ > +var testHeaderVal = "Yes"; > +var testHeaderVal2 = "Very Yes"; > + > + > +function make_channel(url, callback, ctx) { { for function bodies on a new line please. @@ +88,5 @@ > + }, > + > + observe: function(subject, topic, data) { > + if (topic == "http-on-modify-request") { > + if (!(subject instanceof Ci.nsIHttpChannel)) return; you may want to do_throw here instead of return @@ +97,5 @@ > + if (channel.URI.spec == baitURI) target = redirectedURI; > + if (channel.URI.spec == bait2URI) target = redirected2URI; > + if (target) { > + var tURI = ioservice.newURI(target, null, null); > + dump("redirecting request to " + redirectedURI + "\n"); some say dumps in tests are evil. I personally don't but please remove them anyway. @@ +105,5 @@ > + } > + } > +} > + > +var finished=false; finished = false @@ +113,5 @@ > +} > + > +function testViaAsyncOpen () { > + var chan = make_channel(baitURI); > + chan.asyncOpen(new ChannelListener(asyncVerifyCallback),null); , null) @@ +125,5 @@ > + > +function asyncVerifyCallback(req, buffer) { > + dump("in asyncOpen callback\n"); > + if (!(req instanceof Ci.nsIHttpChannel)) > + dump(req + " is not an nsIHttpChannel, catastrophe imminent!"); do_throw here? @@ +127,5 @@ > + dump("in asyncOpen callback\n"); > + if (!(req instanceof Ci.nsIHttpChannel)) > + dump(req + " is not an nsIHttpChannel, catastrophe imminent!"); > + > + var hc = req.QueryInterface(Ci.nsIHttpChannel); var httpChannel @@ +147,5 @@ > + > +function done() { > + dump("done()"); > + httpServer.stop(function () {httpServer2.stop(do_test_finished);}); > +} Move below testViaXHR @@ +149,5 @@ > + dump("done()"); > + httpServer.stop(function () {httpServer2.stop(do_test_finished);}); > +} > + > +function testViaXHR () { function testViaXHR() { @@ +167,5 @@ > +} > + > +function run_test() > +{ > + httpServer = new nsHttpServer(); new HttpServer(); @@ +176,5 @@ > + httpServer2 = new nsHttpServer(); > + httpServer2.registerPathHandler(redirectedPath, redirected2Handler); > + httpServer2.start(4445); > + > + var redirected = new Redirector(); needs to be global (rooting), JS GC could throw it away too soon, since you register it only as a weak reference.

Comment on attachment 680267 [details] [diff] [review] Redirect API patch: fixed Honza's review issues; rebased again to latest m-c Review of attachment 680267 [details] [diff] [review]: ----------------------------------------------------------------- r=honzab but with all comments fixed and with fixed test. Do not land this until the test is up, I will run it locally before approval. I only checked visually since test was not working and it is not enough to approve for landing for me. ::: netwerk/protocol/http/nsHttpChannel.cpp @@ +1559,5 @@ > +NS_IMETHODIMP > +nsHttpChannel::RedirectTo(nsIURI *newURI) > +{ > + // We can only redirect unopened channels > + NS_ENSURE_TRUE(!mWasOpened, NS_ERROR_ALREADY_OPENED); I'm thinking of adding a !mRedirectChannel check too. When redirect is in progress (since we created the target channel and are checking redirect veto) it should not be possible to call this method as well. @@ -1582,5 @@ > } > > return rv; > } > - Leave this line ::: netwerk/protocol/http/nsHttpChannel.h @@ +318,5 @@ > friend class HttpAsyncAborter<nsHttpChannel>; > friend class HttpCacheQuery; > > nsCOMPtr<nsIURI> mRedirectURI; > + nsCOMPtr<nsIURI> mInternalRedirectURI; mInternalRedirectURI is not a proper name. We already have a mechanism called like "internal redirect" which is something different from what you do here. mRedirectToURI or mForcedRedirectToURI are more explanatory and distinctive. Feel free to find even better name. The same applies to all method arguments.

Comment on attachment 680267 [details] [diff] [review] Redirect API patch: fixed Honza's review issues; rebased again to latest m-c Review of attachment 680267 [details] [diff] [review]: ----------------------------------------------------------------- > NS_IMETHODIMP > nsHttpChannel::RedirectTo(nsIURI *newURI) > { > // We can only redirect unopened channels > NS_ENSURE_TRUE(!mWasOpened, NS_ERROR_ALREADY_OPENED); pde/mike, I see from the pastebin you mentioned on IRC that you guys are thinking you need to hook the new http-on-opening-request observer instead of on-modify-request. I'm assuming that's NOT because you need to have it run before proxy resolution, but simply because you otherwise fail this NS_ENSURE_TRUE(!mWasOpened) test above. If that's the case, just change that to NS_ENSURE_TRUE(mRequestObserversCalled, NS_ERROR_ALREADY_OPENED) and AFAICT you'll be fine. And listen to on-modify-request, not on-opening, so you'll get notifications when redirects happen.

Make that NS_ENSURE_TRUE(!mRequestObserversCalled :)

jduell, I've tried the thing you recommend most recently, and it doesn't seem to be working. With the API implemented as NS_ENSURE(!mRequestObserversCalled, NS_ERROR_ALREADY_OPENED), I observe the following: - All redirects fail if I try to do them in response to http-on-modify-request - Most redirects succeed if I try to do them in response to http-on-opening-request, but not redirects that are triggered by another internal redirect My current changes against m-c are here: http://pastebin.com/DVpu6k69 (that's both the API and the test) Note that there's a variable, redirectOpportunity, controlling which nsIObserver topic to watch for, and it's set to "http-on-opening-request", and that Test Part 4 is commented out. Changing either of these things will break the test.

Sorry, the last pastebin link was wrong. After running hg add: http://pastebin.com/8Y6yE2qc

It seems as though my terminal was in some very strange state such that cat-ing that diff caused three lines of it to be missing (!!!). Perhaps my box is pwn3d. Anyway, "reset" fixed this, but I'm giving up and attaching the diff.

Comment on attachment 688964 [details] [diff] [review] Work in progress for jduell It will be a few days before I can look into this. I can't think of a logical reason why we shouldn't be able to do all the redirects within on-modify-request--having to watch on-opening for initial asyncOpens, plus watch on-modify for redirects is kludgy. Hopefully we can make just OMR work.

I performed one last experiment, which was to try making http-on-opening-request fire for all cases, including redirects (see line 118 here: http://pastebin.com/9sCdJyuQ ). That still didn't help with the test part 4, the internal redirect triggered subsequent to a previous internal redirect. I'm willing to try to help further with this, but my C++ and knowledge of the browser's internals are both pretty weak, so I'm not very efficient at this.

Well over in bug #739099 I'm seeing renewed threats to blocklist the 5th most popular Firefox extension because we can't fix and land this patch :(. Given this pressure, my inclination is that we should focus on getting the API to work via http-on-opening-request. It currently does so in all cases except when there's an internal redirect that triggers a second internal redirect.

As we were just discussing on IRC, there appear to be two imperfections with the API patch as currently applicable to m-c: 1. maybe work-aroundable: it only works if you call redirectTo from http-on-opening-request, not http-on-modify-request 2. at least something of a problem: if you try to redirectTo a request that was the result of a previous redirectTo, the second redirect fails A simplified version of the test case which passes is here: https://github.com/pde/httpse-firefox-patches/blob/simplified/test_redirect_from_script.js In order to observe problem #1, change redirectOpportunity to be "http-on-modify-request". That triggers the following error: TEST-UNEXPECTED-FAIL | /home/pde/mozilla-central/obj-x86_64-unknown-linux-gnu/_tests/xpcshell/netwerk/test/unit/test_redirect_from_script.js | Exception in redirectTo [Exception... "Component returned failure code: 0x804b0049 (NS_ERROR_ALREADY_OPENED) [nsIHttpChannel.redirectTo]" nsresult: "0x804b0049 (NS_ERROR_ALREADY_OPENED)" location: "JS frame :: /home/pde/mozilla-central/obj-x86_64-unknown-linux-gnu/_tests/xpcshell/netwerk test/unit/test_redirect_from_script.js :: Redirector.prototype.observe :: line 142" data: no] In order to see problem #2, uncomment line 166/167.

Sorry, this portion was wrong (confused new build environment). The error from problem #1 is just that the tests fail because the redirects aren't happening, as indicated by the absence of the magic header in the response: TEST-UNEXPECTED-FAIL | /home/pde/mozilla-central/obj-x86_64-unknown-linux-gnu/_tests/xpcshell/netwerk/test/unit/test_redirect_from_script.js | null == Yes indeed - See following stack: (In reply to Peter Eckersley from comment #82) > In order to observe problem #1, change redirectOpportunity to be > "http-on-modify-request". That triggers the following error: > > TEST-UNEXPECTED-FAIL | > /home/pde/mozilla-central/obj-x86_64-unknown-linux-gnu/_tests/xpcshell/ > netwerk/test/unit/test_redirect_from_script.js | Exception in redirectTo > [Exception... "Component returned failure code: 0x804b0049 > (NS_ERROR_ALREADY_OPENED) [nsIHttpChannel.redirectTo]" nsresult: > "0x804b0049 (NS_ERROR_ALREADY_OPENED)" location: "JS frame :: > /home/pde/mozilla-central/obj-x86_64-unknown-linux-gnu/_tests/xpcshell/ > netwerk test/unit/test_redirect_from_script.js :: > Redirector.prototype.observe :: line 142" data: no] >

This is just the patch that honza +r'd with a few bitrot fixes. Besides the bustage issue (patches forthcoming) we still need to make Honza's naming changes mentioned in comment 73 (I'm a fan of using 'redirectTo' in the names for everything in this patch: we definitely don't want 'internal').

Simple enough fix, and appears to make all the test code work (new test patch is next up). This only fixes single-process--e10s is still broken.

This is a combo of the original test patch infrastructure, but with the test code from the 'work in progress' patch. pde's version had test commented out--I blindly re-enabled it and it seems to be passing, but I haven't looked at the logic. pde: please make sure this test makes sense to you, and re-submit it for review when you do. Also, we need to add test coverage for calling redirectTo() from a nsIChannelEventSink.asyncOnChannelRedirect() observer (as mentioned as a use case in comment 12: we're still planning to support that, right?).

pde/mperry: you'll need to apply this patch to test e10s (i.e. to run test_redirect_from_script_wrap.js). We turned off on-modify-request for child processes in bug 806753, and this re-enables it. I'm planning to make that re-enabling permanent in bug 827269, but if we can get e10s working here I'm fine with landing without making that bug a dependency for this one (i.e. as long as tests *will* work when that bug lands).

pde/mperry, So test_redirect_from_script_wrap.js still fails with my previous patch (and sadly, with this one too). I thought it was because we were using 'mIPCOpen' in the child to fail RedirectTo() calls--that might fail during a redirect (because the IPDL channel is already open, so mIPCOpen=true), so I changed both nsHttpChannel/HttpChannelChild to use the mRequestObserversCalled instead. But I'm still seeing test 4 failing. I'm also out of time to work on this for now. So your mission is to 1) add test coverage for calling redirectTo from nsIChannelEventSink.asyncOnChannelRedirect(), and 2) try to get everything to work with e10s.

Jason, thanks a ton of helping us get to the bottom of the breakage! I've been staring at NSPR logs for the e10s unit test for a while now, and here's what I think is happening on that front: It looks as if test case 4 (redirect of a redirect) is failing because the e10s child process has no way of communicating the second redirect URI to the parent channel process after the first redirect. In specific: the test script sends the first redirect during OMR, which is called in CallOnModifyRequestObservers() from HttpChannelChild::AsyncOpen(), and the e10s child process sends it to the parent directly as a parameter to SendAsyncOpen(). However, the second time around, nsHttpChannel::AsyncOpen() is then directly called from nsHttpChannel::OpenRedirectChannel() (part of the HSTS/API machinery). While we do appear to get the OMR observer notification in the e10s child for this still, since the e10s child is no longer calling SendAsyncOpen() for this case, the redirect destination is not delivered, and the test fails. This bug was probably introduced by me in the original e10s implementation of the API. The most obvious solution here seems to be also adding an explicit child->parent marshaller + forwarder for the redirectTo call itself back to the parent. Then, we might actually eliminate the redirect parameters for SendAsyncOpen and just let the child set the nsHttpChannel's mInternalRedirectURI through the parent marshaller. But: I'm not sure if this is actually guaranteed to be a valid codepath for e10s, since the nsHttpChannel should be blocking on the return from the observer, but the observer has called the e10s child, which is then doing an IPC back to the parent. Seems like we could risk either deadlock or race conditions this way? I guess I could just test it out and see what happens...

(In reply to Jason Duell (:jduell) from comment #86) > pde: please make sure this test makes sense to you, and re-submit it for > review when you do. It makes sense, Tests 1-4 psas, and I'll re-submit it once I've figured out the answer to this next question: > Also, we need to add test coverage for calling > redirectTo() from a nsIChannelEventSink.asyncOnChannelRedirect() observer > (as mentioned as a use case in comment 12: we're still planning to support > that, right?). Calling newChannel.redirectTo() from asyncOnChannelRedirect does not work at the moment if performed naively (I have an experimental implementation in Tests 5 and 6 here: https://github.com/pde/httpse-firefox-patches/blob/on-channel-redirect/test_redirect_from_script.js though it uses the default ChannelListener, and I'm not sure if that's sufficient, or whether naive == wrong) However, things that NoScript (and by derivation, HTTPS Everywhere) currently does from the asyncOnChannelRedirect hook are significantly more complicated: https://github.com/avian2/noscript/blob/master/xpi/chrome/content/noscript/ChannelReplacement.js https://gitweb.torproject.org/https-everywhere.git/blob/HEAD:/src/chrome/content/code/ChannelReplacement.js This code uses custom ChannelListeners / nsIStreamListeners, and performs the actual redirects via a runWhenPending wrapper, etc. I'd prefer not to reimplement a large subset of that in a unit test unless we need to. In the hope that we don't, we've just started testing HTTPS Everywhere with redirectTo available and asyncOnChannelRedirect completely removed, and it seems to work, but it'll probably be a day or two of testing work before we can have basic confidence in that. Giorgio, what is your opinion about the necessity of the asyncOnChannelRedirect path? Do you know of any reasons why we need to keep it, or cases we should focus on testing?

(In reply to Jason Duell (:jduell) from comment #88) > Created attachment 698636 [details] [diff] [review] > part3, v1: e10s support (still broken) > > pde/mperry, > > So test_redirect_from_script_wrap.js still fails with my previous patch (and > sadly, with this one too). I thought it was because we were using > 'mIPCOpen' in the child to fail RedirectTo() calls--that might fail during a > redirect (because the IPDL channel is already open, so mIPCOpen=true), so I > changed both nsHttpChannel/HttpChannelChild to use the > mRequestObserversCalled instead. But I'm still seeing test 4 failing. Actually I found a bug (fixed here https://github.com/pde/httpse-firefox-patches/commit/4a2117eb401171d0f5a04e4cd8e6e4df6af5a9fb) in test 3, so it is also failing now in e10s (not regular Firefox though). This seems consistent with Mike's theory about what's going wrong in e10s land.

So I tried to make RedirectTo do the Send/Recv parent child remoting thing, and I got an assert in ipc/ipdl/PHttpChannel.cpp, line 28 about an action from a __deleted__ actor. This attachment is that attempt. We're still wondering if we're missing some plumbing here. We noticed for example that request headers properly get sent from the child to the parent in OnRedirectVerifyCallback where jduell added a direct call to the OMR observer, but of course nothing transfers our redirect uri there... Maybe another Send call in that callback would do it?

re: e10s: I don't think you want a new IDPL message for RedirectTo in the child. Instead I suspect you'll want to add a 'redirectToUri' parameter to the Redirect2Verify message, after both the redirect observers and OMR observers on the child have had a chance to set it: http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/HttpChannelChild.cpp#890 I'm guessing that at that point if we have a new redirect (i.e. an observer has decided to redirect the first redirect), we can treat the first redirect as having been vetoed, and then restart the whole e10s redirect process from scratch with the new URI. I don't have time right now to verify that, but hopefully it puts you guys in the right direction. re: testing: from a glance it looks like your EventSink idea is the right way to test the asyncOnChannelRedirect use case (which we should figure out if we really need).

For now you can skip making this code work with e10s. After some discussion it's not clear that we'll ever want to support it on child processes for security reasons. So all we need to do here is figure out if we need the asyncOnChannelRedirect use case, and if so, fix it. Otherwise looks like we're very close.

Heh--actually turns out it would be nice to have e10s support here-we'll probably want it eventually. But I'm ok with landing this with redirectTo throwing an error on the child, and we can get it working in a followup.

(In reply to Jason Duell (:jduell) from comment #95) > Heh--actually turns out it would be nice to have e10s support here-we'll > probably want it eventually. But I'm ok with landing this with redirectTo > throwing an error on the child, and we can get it working in a followup. Yes, I agree, let's move forward here w/o further complications. Does it mean we don't have any more pending issues here and I can do the review now?

(In reply to Honza Bambas (:mayhemer) from comment #96) > Does it mean we don't have any more pending issues here and I can do the > review now? I think we'll have a patch for you to review in 1-2 hours.

Mike just got a version of the patch to work in e10s, but it depends on jduell's patch that reverses 806753. So unless someone tells us otherwise, what we'll put together is a version that's commented out in e10s and has the accompanying e10s unit test disabled, but which is ready to go once the http-on-* observers are reenabled in electrolysis child processes.

I've fixed Honza's concerns about the "internal" naming confusion and the whitespace issue. I did not add the mRedirectChannel to the failure check, since jduell moved the RedirectTo implementation to HttpBaseChannel where mRedirectChannel is not available.. I've also left the e10s support in, since it does work if OMR is available. I figure we can just disable the unit test for e10s until such time as e10s OMR is available again. I'm also not allowed to obsolete jduell's patches, but this patch does contain his initial e10s changes and bitrot fixes as well.

(In reply to Peter Eckersley from comment #90) > Giorgio, what is your opinion about the necessity of the > asyncOnChannelRedirect path? Do you know of any reasons why we need to keep > it, or cases we should focus on testing? As far as I can tell being able to perform redirectTo() from http-on-modify-request observers should cover any conceivable use case (unless we wanted to redirect non-HTTP requests as well, but in that case I'm afraid there's no suitable callback yet). NoScript listens for asyncOnChannelRedirect mostly because nsIContentPolicy doesn't get called on redirection (in fact, we just force call shouldLoad() on our content policy from there). After redirections to any HTTP destination, http-on-modify-request gets called anyway, and this should suffice for redirectTo() purposes.

pde pointed out that I needed to ensure that the observer calls need to stay commented. This patch should now apply cleanly without the OMR backout patch, but the e10s tests will now fail because the OMR calls are commented out. We're going to comment out the test cases for e10s, and then create an additional patch that turns the observers and the e10s tests back on.

Okay, here is what should be the final unit test cases for the API. e10s tests are present but disabled until 827269 is resolved.

(In reply to Mike Perry from comment #99) > Created attachment 700055 [details] [diff] [review] > Fully working channel.redirectTo API implementation; includes e10s support. It's missing functions context (again). > > I've fixed Honza's concerns about the "internal" naming confusion and the > whitespace issue. I did not add the mRedirectChannel to the failure check, > since jduell moved the RedirectTo implementation to HttpBaseChannel where > mRedirectChannel is not available.. mRedirectChannel can be non-null only after mRequestObserversCalled is true. So you are safe with your check in RedirectTo.

hg diff -U 10 -p...

Comment on attachment 700156 [details] [diff] [review] channel.redirectTo API; leave OMR commented out for e10s (which breaks it). Flagging the version with function context for review.

Comment on attachment 700090 [details] [diff] [review] channel.redirectTo API; leave OMR commented out for e10s (which breaks it). Please don't obsolete patches that people could potentially start writing splinter comments. You almost made them lost.

I'll locally run some base tests to check we don't break things with this patch and then will push to try if local tests go well.

"make check" passes, but this is what I'm seeing from xpcshell tests :( http://pastebin.com/VW6UbJNq The six unit_ipc failures seem to be caused by yesterday's patch (test_bug455311.js fails in m-c without our help). Sorry for not running this before uploading.

Comment on attachment 700156 [details] [diff] [review] channel.redirectTo API; leave OMR commented out for e10s (which breaks it). This patch doesn't apply cleanly to current m-c. Use strictly context of 8 lines please, different context size makes comparing patches very difficult and is not according mozilla patch submit rules. pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=c2dfd5ded9e1 PLEASE DON'T OBSOLETE ANY PATCHES, THANKS.

Comment on attachment 700090 [details] [diff] [review] channel.redirectTo API; leave OMR commented out for e10s (which breaks it). Review of attachment 700090 [details] [diff] [review]: ----------------------------------------------------------------- r=honzab with the comments addressed before landing. ::: netwerk/protocol/http/HttpBaseChannel.cpp @@ +1159,5 @@ > +NS_IMETHODIMP > +HttpBaseChannel::RedirectTo(nsIURI *newURI) > +{ > + // We can only redirect unopened channels > + NS_ENSURE_TRUE(!mRequestObserversCalled, NS_ERROR_ALREADY_OPENED); You have ENSURE_CALLED_BEFORE_CONNECT() macro. ::: netwerk/protocol/http/PHttpChannel.ipdl @@ +37,5 @@ > // chrome channel would ever need to know. Get rid of next arg? > OptionalURIParams original, > OptionalURIParams doc, > OptionalURIParams referrer, > + OptionalURIParams internalRedirect, redirectTo or what the name consensus it. ::: netwerk/protocol/http/nsIHttpChannel.idl @@ +267,5 @@ > + * > + * @throws NS_ERROR_ALREADY_OPENED if called after the channel > + * has been opened. > + */ > + void redirectTo(in nsIURI aNewURI); Change also IID !

Comment on attachment 700135 [details] [diff] [review] Final test cases for the redirectTo API Review of attachment 700135 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/test/unit/test_redirect_from_script.js @@ +29,5 @@ > +const Cr = Components.results; > + > +Cu.import("resource://testing-common/httpd.js"); > + > +redirectOpportunity = "http-on-modify-request"; s/redirectOpportunity/onModifyRequestTopic/ ? @@ +61,5 @@ > +var bait4URI = "http://localhost:4444" + bait4Path; > + > +var testHeaderName = "X-Redirected-By-Script" > +var testHeaderVal = "Yes indeed"; > +var testHeaderVal2 = "Very Yes"; The names and values are funny but hard to follow the logic with them which URI leads to what and what is the final target URI, etc. But this is your code. @@ +134,5 @@ > +  // if we have a target, redirect there > + if (target) { > + var tURI = ioservice.newURI(target, null, null); > + try { channel.redirectTo(tURI); } > + catch (e) { do_throw("Exception in redirectTo " + e + "\n"); } try { } catch (e) { } @@ +140,5 @@ > + } > + } > +} > + > +finished=false; ' = ' @@ +145,5 @@ > + > +function Redirector() > +{ > + this.register(); > +} Above the prototype please. @@ +185,5 @@ > +testViaAsyncOpen3 = makeAsyncOpenTest(bait3URI, asyncVerifyCallback3); > +asyncVerifyCallback2 = makeVerifier (testHeaderVal2, testViaAsyncOpen3); > +testViaAsyncOpen2 = makeAsyncOpenTest(bait2URI, asyncVerifyCallback2); > +asyncVerifyCallback = makeVerifier (testHeaderVal, testViaAsyncOpen2); > +testViaAsyncOpen = makeAsyncOpenTest(baitURI, asyncVerifyCallback); 'var' all of these. You can also: var testViaAsyncOpen = makeAsyncOpenTest(baitURI, makeVerifier(testHeaderVal, makeAsyncOpenTest(bait2URI, makeVerifier (testHeaderVal2, etc... it could be more readable. You can also create the verifier inside makeAsyncOpenTest(uri, headerValue, next). Even simpler! @@ +210,5 @@ > +} > + > +function done() > +{ > + httpServer.stop(function () {httpServer2.stop(do_test_finished);}); { httpServer2.stop(do_test_finished); } please @@ +226,5 @@ > + httpServer2 = new HttpServer(); > + httpServer2.registerPathHandler(redirectedPath, redirected2Handler); > + httpServer2.start(4445); > + > + redirected = new Redirector(); this should be global (with var) and called redirector.

Late last night pde's server finally finished churning through all of the other unit tests and he noticed that test_redirect_canceled_wrap.js was broken by our patch. This new patch fixes that unit test (with an else clause added to HttpChannelChild::OnRedirectVerifyCallback to send an empty API uri for canceled redirects), and addresses Honza's review comments for the C++/API portion.

(In reply to Honza Bambas (:mayhemer) from comment #110) > PLEASE DON'T OBSOLETE ANY PATCHES, THANKS. Why not?

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #114) > (In reply to Honza Bambas (:mayhemer) from comment #110) > > PLEASE DON'T OBSOLETE ANY PATCHES, THANKS. > > Why not? Now you can. But splinter drafts gets cleared when a patch is obsoleted.

Sounds like a bug in splinter. Is it filed?

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #116) > Sounds like a bug in splinter. Is it filed? Not sure. I'll check the bug is still present and report.

Note for me: push the latest patch to try.

(In reply to Honza Bambas (:mayhemer) from comment #119) > https://tbpl.mozilla.org/?tree=Try&rev=9c9ee2d892d3 No failures, but are the timeouts and infrastructure exception indicative of things that we need to debug, or is this common and unproblematic?

(In reply to Peter Eckersley from comment #120) > (In reply to Honza Bambas (:mayhemer) from comment #119) > > https://tbpl.mozilla.org/?tree=Try&rev=9c9ee2d892d3 > > No failures, but are the timeouts and infrastructure exception indicative of > things that we need to debug, or is this common and unproblematic? No. The patch seems OK.

Here is an updated test changset, containing the exact patch that Honza reviewed in Comment 112, and an extra commit for the refactoring and neatening he suggested there.

Comment on attachment 705989 [details] [diff] [review] Final API test cases + neatening per Honza's r+ review better patch is coming

Previous patch but squashed into a single commit.

Honza: bz didn't reply when I pinged him, so I presume he's busy. I think we should just land this!

I agree we don't need another sr for a simple name change. But do the current patches rely on the reverse patch of bug 806753? I should probably apply that patch in bug 827269 and then we can land this with e10s support and testing. I'll try to get to that today.

(In reply to Jason Duell (:jduell) from comment #126) > I agree we don't need another sr for a simple name change. > > But do the current patches rely on the reverse patch of bug 806753? I > should probably apply that patch in bug 827269 and then we can land this > with e10s support and testing. I'll try to get to that today. These patches do not depend on that stuff; the API and tests are disabled in e10s. We can land these patches in m-c now and then later (even later the same day ;)) apply something like the patch I attached to bug 828739.

(In reply to Peter Eckersley from comment #127) > the API and tests are disabled in > e10s Slight correction after re-reading bug 828739: the API is present but it won't work because the observers one needs to call it from are disabled.

OK, so we can land w/o e10s support for now, i.e. no bug 827269 needed.

Comment on attachment 701346 [details] [diff] [review] nsIHttpChannel.redirectTo API; fixes cancel unit test breakage Review of attachment 701346 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/protocol/http/HttpBaseChannel.cpp @@ +1156,5 @@ > return NS_OK; > } > > +NS_IMETHODIMP > +HttpBaseChannel::RedirectTo(nsIURI *newURI) Let's add if (IsNeckoChild()) { return NS_ERROR_NOT_IMPLEMENTED } to the beginning of this function as we don't actually support it in the child for now.

Actually better still would be to implement the RedirectTo in nsHttpChannel and HttpChannelChild separately, with the latter just having the code in the last comment. And remove the HttpBaseChannel impl.

https://hg.mozilla.org/integration/mozilla-inbound/rev/81283622f4da https://hg.mozilla.org/integration/mozilla-inbound/rev/32ca51d581bf \o/

https://hg.mozilla.org/mozilla-central/rev/81283622f4da https://hg.mozilla.org/mozilla-central/rev/32ca51d581bf

This fix should be uplifted at least to aurora (Firefox 20), and arguably to earlier Firefox versions. It does contain a change to IDL, but that change does not carry the typical risks of an API change because it merely adds a new API function which is not currently called by any code in the wild besides HTTPS Everywhere (which calls it if and only if it's present). On IRC jduell and mayhemer may have reached the conclusion that the patch might therefore be upliftable without a UUID change, although they should confirm that. As for why uplift is important, my belief is that this patch fixes most or all of the following security and extension-incompatibility bugs: https://trac.torproject.org/projects/tor/ticket/3190 (further exposition at http://lduros.net/posts/https-everywhere-and-xhr-other-add-ons/ ) https://trac.torproject.org/projects/tor/ticket/6499 https://trac.torproject.org/projects/tor/ticket/6592 https://trac.torproject.org/projects/tor/ticket/6688 https://trac.torproject.org/projects/tor/ticket/7769 https://trac.torproject.org/projects/tor/ticket/5477 https://bugzilla.mozilla.org/show_bug.cgi?id=739099

Comment on attachment 701346 [details] [diff] [review] nsIHttpChannel.redirectTo API; fixes cancel unit test breakage See previous comment for general justification here. I'd be fine taking this on aurora (uuid issue may need resolving: can we either change uuid on aurora, or keep existing uuid if we've only added a function to the IDL? I'd hope the latter). It's unlikely to break existing codepaths, so maybe beta? [Approval Request Comment] User impact if declined: various TOR security bugs (see prev comment) Testing completed (on m-c, etc.): on mc with test. Risk to taking this patch (and alternatives if risky): risk to code that doesn't call new codepath appears minimal if any. String or UUID changes made by this patch: none

(In reply to Jason Duell (:jduell) from comment #135) > User impact if declined: various TOR security bugs (see prev comment) And for clarity: these compatibility and security bugs do not just affect Tor users; they affect several million other HTTPS Everywhere users as well. It's just that we happen to use Tor's bug tracker for HTTPS Everywhere bugs.

Benjamin, can you chime in on the UUID question from comment 134 and comment 135?

(In reply to Jason Duell (:jduell) from comment #135) > User impact if declined: various TOR security bugs (see prev comment) To be clear: These are bugs caused by a bug in the addons. They are not caused by bugs in Gecko. I am happy to help on the Gecko side, as long as that help doesn't create problems. But, generally, bugs in addon code--including security bugs--should be resolved within the addon, because sometimes it will not be practical for us to change Gecko to deal with the addons' bugs. In this case, I am concerned that nsIHttpChannel may too commonly used to change its UUID this close to a release. We risk creating instability in binary addons that have already verified their addon works correctly against the previous four weeks of Firefox beta builds. IIRC, we've got burned changing this interface in beta releases before.

We can land UUID changes on aurora. Jorge should be aware of them but in general it's not a problem. We should not take any UUID change after beta1, which is the SDK that binary addons typically compile against; nor should we change the interface without an IID change because of the possibility that there are derived interfaces that would end up changed by accident.

Comment on attachment 701346 [details] [diff] [review] nsIHttpChannel.redirectTo API; fixes cancel unit test breakage Approving for Aurora given our suspicions in https://bugzilla.mozilla.org/show_bug.cgi?id=765934#c134, but it's too late in Beta to be taking a large change, or for that matter, an interface change.

https://hg.mozilla.org/releases/mozilla-aurora/rev/237990d8bcba https://hg.mozilla.org/releases/mozilla-aurora/rev/30c3a5df665c

https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/21 https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsIHttpChannel

(In reply to Kris Maglione [:kmag] from comment #142) > https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/21 I think this should have been in the Firefox 20 changelog, actually ( https://mxr.mozilla.org/mozilla-release/ident?i=RedirectTo&tree=mozilla-release&filter=netwerk%2Fprotocol ) > https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsIHttpChannel Edited.

Hm. You're right. The target milestone is set to 21. I didn't realize it had been uplifted.

Seems we might need to uplift this to the b2g branches for bug 852848 (not clear yet, so not marking as dependency). Not that big of a patch.

missed a qref. Doesn't look like we'll need this on b2g18 branch anyway...