Bug 1231512 - Allow nsIHttpChannel.redirectTo() work also on an open channel, r=jduell

netwerk/protocol/http/HttpBaseChannel.cpp

@@ -1778,14 +1778,15 @@ HttpBaseChannel::GetRequestSucceeded(bool *aValue)
 }
 
 NS_IMETHODIMP
-HttpBaseChannel::RedirectTo(nsIURI *newURI)
+HttpBaseChannel::RedirectTo(nsIURI *targetURI)
 {
-  // We can only redirect unopened channels
-  ENSURE_CALLED_BEFORE_CONNECT();
-
-  // The redirect is stored internally for use in AsyncOpen
-  mAPIRedirectToURI = newURI;
+  // We cannot redirect after OnStartRequest of the listener
+  // has been called, since to redirect we have to switch channels
+  // and the dance with OnStartRequest et al has to start over.
+  // This would break the nsIStreamListener contract.
+  NS_ENSURE_FALSE(mOnStartRequestCalled, NS_ERROR_NOT_AVAILABLE);
 
+  mAPIRedirectToURI = targetURI;
   return NS_OK;
 }
 

netwerk/protocol/http/nsHttpChannel.cpp

@@ -1568,6 +1568,39 @@ nsHttpChannel::ProcessResponse()
         LOG(("  continuation state has been reset"));
     }
 
+    if (mAPIRedirectToURI && !mCanceled) {
+        MOZ_ASSERT(!mOnStartRequestCalled);
+        nsCOMPtr<nsIURI> redirectTo;
+        mAPIRedirectToURI.swap(redirectTo);
+
+        PushRedirectAsyncFunc(&nsHttpChannel::ContinueProcessResponse1);
+        rv = StartRedirectChannelToURI(redirectTo, nsIChannelEventSink::REDIRECT_TEMPORARY);
+        if (NS_SUCCEEDED(rv)) {
+            return NS_OK;
+        }
+        PopRedirectAsyncFunc(&nsHttpChannel::ContinueProcessResponse1);
+    }
+
+    // Hack: ContinueProcessResponse1 uses NS_OK to detect successful
+    // redirects, so we distinguish this codepath (a non-redirect that's
+    // processing normally) by passing in a bogus error code.
+    return ContinueProcessResponse1(NS_BINDING_FAILED);
+}
+
+nsresult
+nsHttpChannel::ContinueProcessResponse1(nsresult rv)
+{
+    if (NS_SUCCEEDED(rv)) {
+        // redirectTo() has passed through, we don't want to go on with
+        // this channel.  It will now be canceled by the redirect handling
+        // code that called this function.
+        return NS_OK;
+    }
+
+    rv = NS_OK;
+
+    uint32_t httpStatus = mResponseHead->Status();
+
     bool successfulReval = false;
 
     // handle different server response categories.  Note that we handle
@@ -1610,10 +1643,10 @@ nsHttpChannel::ProcessResponse()
 #endif
         // don't store the response body for redirects
         MaybeInvalidateCacheEntryForSubsequentGet();
-        PushRedirectAsyncFunc(&nsHttpChannel::ContinueProcessResponse);
+        PushRedirectAsyncFunc(&nsHttpChannel::ContinueProcessResponse2);
         rv = AsyncProcessRedirection(httpStatus);
         if (NS_FAILED(rv)) {
-            PopRedirectAsyncFunc(&nsHttpChannel::ContinueProcessResponse);
+            PopRedirectAsyncFunc(&nsHttpChannel::ContinueProcessResponse2);
             LOG(("AsyncProcessRedirection failed [rv=%x]\n", rv));
             // don't cache failed redirect responses.
             if (mCacheEntry)
@@ -1622,7 +1655,7 @@ nsHttpChannel::ProcessResponse()
                 mStatus = rv;
                 DoNotifyListener();
             } else {
-                rv = ContinueProcessResponse(rv);
+                rv = ContinueProcessResponse2(rv);
             }
         }
         break;
@@ -1697,7 +1730,7 @@ nsHttpChannel::ProcessResponse()
 }
 
 nsresult
-nsHttpChannel::ContinueProcessResponse(nsresult rv)
+nsHttpChannel::ContinueProcessResponse2(nsresult rv)
 {
     bool doNotRender = DoNotRender3xxBody(rv);
 
@@ -1713,7 +1746,7 @@ nsHttpChannel::ContinueProcessResponse(nsresult rv)
             // redirecting to another protocol (perhaps javascript:)
             // In that case we want to throw an error instead of displaying the
             // non-redirected response body.
-            LOG(("ContinueProcessResponse detected rejected Non-HTTP Redirection"));
+            LOG(("ContinueProcessResponse2 detected rejected Non-HTTP Redirection"));
             doNotRender = true;
             rv = NS_ERROR_CORRUPTED_CONTENT;
         }
@@ -1739,7 +1772,7 @@ nsHttpChannel::ContinueProcessResponse(nsresult rv)
         return NS_OK;
     }
 
-    LOG(("ContinueProcessResponse got failure result [rv=%x]\n", rv));
+    LOG(("ContinueProcessResponse2 got failure result [rv=%x]\n", rv));
     if (mTransaction->ProxyConnectFailed()) {
         return ProcessFailedProxyConnect(mRedirectType);
     }
@@ -2009,8 +2042,13 @@ nsHttpChannel::StartRedirectChannelToURI(nsIURI *upgradedURI, uint32_t flags)
 nsresult
 nsHttpChannel::ContinueAsyncRedirectChannelToURI(nsresult rv)
 {
-    if (NS_SUCCEEDED(rv))
+    // Since we handle mAPIRedirectToURI also after on-examine-response handler
+    // rather drop it here to avoid any redirect loops, even just hypothetical.
+    mAPIRedirectToURI = nullptr;
+
+    if (NS_SUCCEEDED(rv)) {
         rv = OpenRedirectChannel(rv);
+    }
 
     if (NS_FAILED(rv)) {
         // Fill the failure status here, the update to https had been vetoed
@@ -2019,8 +2057,9 @@ nsHttpChannel::ContinueAsyncRedirectChannelToURI(nsresult rv)
         mStatus = rv;
     }
 
-    if (mLoadGroup)
+    if (mLoadGroup) {
         mLoadGroup->RemoveRequest(this, nullptr, mStatus);
+    }
 
     if (NS_FAILED(rv)) {
         // We have to manually notify the listener because there is not any pump
@@ -5703,6 +5742,8 @@ nsHttpChannel::OnStartSignedPackageRequest(const nsACString& aPackageId)
 NS_IMETHODIMP
 nsHttpChannel::OnStartRequest(nsIRequest *request, nsISupports *ctxt)
 {
+    nsresult rv;
+
     PROFILER_LABEL("nsHttpChannel", "OnStartRequest",
         js::ProfileEntry::Category::NETWORK);
 
@@ -5757,35 +5798,70 @@ nsHttpChannel::OnStartRequest(nsIRequest *request, nsISupports *ctxt)
         return NS_OK;
     }
 
+    // before we start any content load, check for redirectTo being called
+    // this code is executed mainly before we start load from the cache
+    if (mAPIRedirectToURI && !mCanceled) {
+        nsAutoCString redirectToSpec;
+        mAPIRedirectToURI->GetAsciiSpec(redirectToSpec);
+        LOG(("  redirectTo called with uri=%s", redirectToSpec.BeginReading()));
+
+        MOZ_ASSERT(!mOnStartRequestCalled);
+
+        nsCOMPtr<nsIURI> redirectTo;
+        mAPIRedirectToURI.swap(redirectTo);
+
+        PushRedirectAsyncFunc(&nsHttpChannel::ContinueOnStartRequest1);
+        rv = StartRedirectChannelToURI(redirectTo, nsIChannelEventSink::REDIRECT_TEMPORARY);
+        if (NS_SUCCEEDED(rv)) {
+            return NS_OK;
+        }
+        PopRedirectAsyncFunc(&nsHttpChannel::ContinueOnStartRequest1);
+    }
+
+    // Hack: ContinueOnStartRequest1 uses NS_OK to detect successful redirects,
+    // so we distinguish this codepath (a non-redirect that's processing
+    // normally) by passing in a bogus error code.
+    return ContinueOnStartRequest1(NS_BINDING_FAILED);
+}
+
+nsresult
+nsHttpChannel::ContinueOnStartRequest1(nsresult result)
+{
+    if (NS_SUCCEEDED(result)) {
+        // Redirect has passed through, we don't want to go on with this
+        // channel.  It will now be canceled by the redirect handling code
+        // that called this function.
+        return NS_OK;
+    }
+
     // on proxy errors, try to failover
     if (mConnectionInfo->ProxyInfo() &&
        (mStatus == NS_ERROR_PROXY_CONNECTION_REFUSED ||
         mStatus == NS_ERROR_UNKNOWN_PROXY_HOST ||
         mStatus == NS_ERROR_NET_TIMEOUT)) {
 
-        PushRedirectAsyncFunc(&nsHttpChannel::ContinueOnStartRequest1);
+        PushRedirectAsyncFunc(&nsHttpChannel::ContinueOnStartRequest2);
         if (NS_SUCCEEDED(ProxyFailover()))
             return NS_OK;
-        PopRedirectAsyncFunc(&nsHttpChannel::ContinueOnStartRequest1);
+        PopRedirectAsyncFunc(&nsHttpChannel::ContinueOnStartRequest2);
     }
 
-    return ContinueOnStartRequest2(NS_OK);
-}
-
-nsresult
-nsHttpChannel::ContinueOnStartRequest1(nsresult result)
-{
-    // Success indicates we passed ProxyFailover, in that case we must not continue
-    // with this code chain.
-    if (NS_SUCCEEDED(result))
-        return NS_OK;
-
-    return ContinueOnStartRequest2(result);
+    // Hack: ContinueOnStartRequest2 uses NS_OK to detect successful redirects,
+    // so we distinguish this codepath (a non-redirect that's processing
+    // normally) by passing in a bogus error code.
+    return ContinueOnStartRequest2(NS_BINDING_FAILED);
 }
 
 nsresult
 nsHttpChannel::ContinueOnStartRequest2(nsresult result)
 {
+    if (NS_SUCCEEDED(result)) {
+        // Redirect has passed through, we don't want to go on with this
+        // channel.  It will now be canceled by the redirect handling code
+        // that called this function.
+        return NS_OK;
+    }
+
     // on other request errors, try to fall back
     if (NS_FAILED(mStatus)) {
         PushRedirectAsyncFunc(&nsHttpChannel::ContinueOnStartRequest3);

netwerk/protocol/http/nsHttpChannel.h

@@ -272,7 +272,8 @@ private:
     void     SetupTransactionSchedulingContext();
     nsresult CallOnStartRequest();
     nsresult ProcessResponse();
-    nsresult ContinueProcessResponse(nsresult);
+    nsresult ContinueProcessResponse1(nsresult);
+    nsresult ContinueProcessResponse2(nsresult);
     nsresult ProcessNormal();
     nsresult ContinueProcessNormal(nsresult);
     void     ProcessAltService();

netwerk/protocol/http/nsIHttpChannel.idl

@@ -370,15 +370,33 @@ interface nsIHttpChannel : nsIChannel
 
     /**
      * Instructs the channel to immediately redirect to a new destination.
-     * Can only be called on channels not yet opened.
+     * Can only be called on channels that have not yet called their
+     * listener's OnStartRequest(). Generally that means the latest time
+     * this can be used is one of:
+     *    "http-on-examine-response"
+     *    "http-on-examine-merged-response"
+     *    "http-on-examine-cached-response"
+     *
+     * When non-null URL is set before AsyncOpen:
+     *  we attempt to redirect to the targetURI before we even start building
+     *  and sending the request to the cache or the origin server.
+     *  If the redirect is vetoed, we fail the channel.
+     *
+     * When set between AsyncOpen and first call to OnStartRequest being called:
+     *  we attempt to redirect before we start delivery of network or cached
+     *  response to the listener.  If vetoed, we continue with delivery of
+     *  the original content to the channel listener.
+     *
+     * When passed aTargetURI is null the channel behaves normally (can be
+     * rewritten).
      *
      * This method provides no explicit conflict resolution. The last
      * caller to call it wins.
      *
-     * @throws NS_ERROR_ALREADY_OPENED if called after the channel
-     *         has been opened.
+     * @throws NS_ERROR_NOT_AVAILABLE if called after the channel has already
+     *         started to deliver the content to its listener.
      */
-    void redirectTo(in nsIURI aNewURI);
+    void redirectTo(in nsIURI aTargetURI);
 
     /**
      * Identifies the scheduling context for this load.

netwerk/test/unit/test_redirect_from_script_after-open_passing.js

@@ -0,0 +1,258 @@
+/*
+ * Test whether the rewrite-requests-from-script API implemented here:
+ * https://bugzilla.mozilla.org/show_bug.cgi?id=765934 is functioning
+ * correctly
+ *
+ * The test has the following components:
+ *
+ * testViaXHR() checks that internal redirects occur correctly for requests
+ * made with nsIXMLHttpRequest objects.
+ *
+ * testViaAsyncOpen() checks that internal redirects occur correctly when made
+ * with nsIHTTPChannel.asyncOpen2().
+ *
+ * Both of the above functions tests four requests:
+ *
+ * Test 1: a simple case that redirects within a server;
+ * Test 2: a second that redirects to a second webserver;
+ * Test 3: internal script redirects in response to a server-side 302 redirect;
+ * Test 4: one internal script redirects in response to another's redirect.
+ *
+ * The successful redirects are confirmed by the presence of a custom response
+ * header.
+ *
+ */
+
+Cu.import("resource://testing-common/httpd.js");
+Cu.import("resource://gre/modules/NetUtil.jsm");
+
+// the topic we observe to use the API.  http-on-opening-request might also
+// work for some purposes.
+redirectHook = "http-on-examine-response";
+
+var httpServer = null, httpServer2 = null;
+
+XPCOMUtils.defineLazyGetter(this, "port1", function() {
+  return httpServer.identity.primaryPort;
+});
+
+XPCOMUtils.defineLazyGetter(this, "port2", function() {
+  return httpServer2.identity.primaryPort;
+});
+
+// Test Part 1: a cross-path redirect on a single HTTP server
+// http://localhost:port1/bait -> http://localhost:port1/switch
+var baitPath = "/bait";
+XPCOMUtils.defineLazyGetter(this, "baitURI", function() {
+  return "http://localhost:" + port1 + baitPath;
+});
+var baitText = "you got the worm";
+
+var redirectedPath = "/switch";
+XPCOMUtils.defineLazyGetter(this, "redirectedURI", function() {
+  return "http://localhost:" + port1 + redirectedPath;
+});
+var redirectedText = "worms are not tasty";
+
+// Test Part 2: Now, a redirect to a different server
+// http://localhost:port1/bait2 -> http://localhost:port2/switch
+var bait2Path = "/bait2";
+XPCOMUtils.defineLazyGetter(this, "bait2URI", function() {
+  return "http://localhost:" + port1 + bait2Path;
+});
+
+XPCOMUtils.defineLazyGetter(this, "redirected2URI", function() {
+  return "http://localhost:" + port2 + redirectedPath;
+});
+
+// Test Part 3, begin with a serverside redirect that itself turns into an instance
+// of Test Part 1
+var bait3Path = "/bait3";
+XPCOMUtils.defineLazyGetter(this, "bait3URI", function() {
+  return "http://localhost:" + port1 + bait3Path;
+});
+
+// Test Part 4, begin with this client-side redirect and which then redirects
+// to an instance of Test Part 1
+var bait4Path = "/bait4";
+XPCOMUtils.defineLazyGetter(this, "bait4URI", function() {
+  return "http://localhost:" + port1 + bait4Path;
+});
+
+var testHeaderName = "X-Redirected-By-Script"
+var testHeaderVal = "Success";
+var testHeaderVal2 = "Success on server 2";
+
+function make_channel(url, callback, ctx) {
+  return NetUtil.newChannel({uri: url, loadUsingSystemPrincipal: true});
+}
+
+function baitHandler(metadata, response)
+{
+  // Content-Type required: https://bugzilla.mozilla.org/show_bug.cgi?id=748117
+  response.setHeader("Content-Type", "text/html", false);
+  response.bodyOutputStream.write(baitText, baitText.length);
+}
+
+function redirectedHandler(metadata, response)
+{
+  response.setHeader("Content-Type", "text/html", false);
+  response.bodyOutputStream.write(redirectedText, redirectedText.length);
+  response.setHeader(testHeaderName, testHeaderVal);
+}
+
+function redirected2Handler(metadata, response)
+{
+  response.setHeader("Content-Type", "text/html", false);
+  response.bodyOutputStream.write(redirectedText, redirectedText.length);
+  response.setHeader(testHeaderName, testHeaderVal2);
+}
+
+function bait3Handler(metadata, response)
+{
+  response.setHeader("Content-Type", "text/html", false);
+  response.setStatusLine(metadata.httpVersion, 302, "Found");
+  response.setHeader("Location", baitURI);
+}
+
+function Redirector()
+{
+  this.register();
+}
+
+Redirector.prototype = {
+  // This class observes an event and uses that to
+  // trigger a redirectTo(uri) redirect using the new API
+  register: function()
+  {
+    Cc["@mozilla.org/observer-service;1"].
+      getService(Ci.nsIObserverService).
+      addObserver(this, redirectHook, true);
+  },
+
+  QueryInterface: function(iid)
+  {
+    if (iid.equals(Ci.nsIObserver) ||
+        iid.equals(Ci.nsISupportsWeakReference) ||
+        iid.equals(Ci.nsISupports))
+      return this;
+    throw Components.results.NS_NOINTERFACE;
+  },
+
+  observe: function(subject, topic, data)
+  {
+    if (topic == redirectHook) {
+      if (!(subject instanceof Ci.nsIHttpChannel))
+        do_throw(redirectHook + " observed a non-HTTP channel");
+      var channel = subject.QueryInterface(Ci.nsIHttpChannel);
+      var ioservice = Cc["@mozilla.org/network/io-service;1"].
+                        getService(Ci.nsIIOService);
+      var target = null;
+      if (channel.URI.spec == baitURI)  target = redirectedURI;
+      if (channel.URI.spec == bait2URI) target = redirected2URI;
+      if (channel.URI.spec == bait4URI) target = baitURI;
+       // 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");
+        }
+      }
+    }
+  }
+}
+
+function makeAsyncTest(uri, headerValue, nextTask)
+{
+  // Make a test to check a redirect that is created with channel.asyncOpen2()
+
+  // Produce a callback function which checks for the presence of headerValue,
+  // and then continues to the next async test task
+  var verifier = function(req, buffer)
+  {
+    if (!(req instanceof Ci.nsIHttpChannel))
+      do_throw(req + " is not an nsIHttpChannel, catastrophe imminent!");
+
+    var httpChannel = req.QueryInterface(Ci.nsIHttpChannel);
+    do_check_eq(httpChannel.getResponseHeader(testHeaderName), headerValue);
+    do_check_eq(buffer, redirectedText);
+    nextTask();
+  };
+
+  // Produce a function to run an asyncOpen2 test using the above verifier
+  var test = function()
+  {
+    var chan = make_channel(uri);
+    chan.asyncOpen2(new ChannelListener(verifier));
+  };
+  return test;
+}
+
+// will be defined in run_test because of the lazy getters,
+// since the server's port is defined dynamically
+var testViaAsyncOpen4 = null;
+var testViaAsyncOpen3 = null;
+var testViaAsyncOpen2 = null;
+var testViaAsyncOpen  = null;
+
+function testViaXHR()
+{
+  runXHRTest(baitURI,  testHeaderVal);
+  runXHRTest(bait2URI, testHeaderVal2);
+  runXHRTest(bait3URI, testHeaderVal);
+  runXHRTest(bait4URI, testHeaderVal);
+}
+
+function runXHRTest(uri, headerValue)
+{
+  // Check that making an XHR request for uri winds up redirecting to a result with the
+  // appropriate headerValue
+  var xhr = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"];
+
+  var req = xhr.createInstance(Ci.nsIXMLHttpRequest);
+  req.open("GET", uri, false);
+  req.send();
+  do_check_eq(req.getResponseHeader(testHeaderName), headerValue);
+  do_check_eq(req.response, redirectedText);
+}
+
+function done()
+{
+  httpServer.stop(
+    function ()
+    {
+      httpServer2.stop(do_test_finished);
+    }
+  );
+}
+
+var redirector = new Redirector();
+
+function run_test()
+{
+  httpServer = new HttpServer();
+  httpServer.registerPathHandler(baitPath, baitHandler);
+  httpServer.registerPathHandler(bait2Path, baitHandler);
+  httpServer.registerPathHandler(bait3Path, bait3Handler);
+  httpServer.registerPathHandler(bait4Path, baitHandler);
+  httpServer.registerPathHandler(redirectedPath, redirectedHandler);
+  httpServer.start(-1);
+  httpServer2 = new HttpServer();
+  httpServer2.registerPathHandler(redirectedPath, redirected2Handler);
+  httpServer2.start(-1);
+
+  // The tests depend on each other, and therefore need to be defined in the
+  // reverse of the order they are called in.  It is therefore best to read this
+  // stanza backwards!
+  testViaAsyncOpen4 = makeAsyncTest(bait4URI, testHeaderVal, done);
+  testViaAsyncOpen3 = makeAsyncTest(bait3URI, testHeaderVal, testViaAsyncOpen4);
+  testViaAsyncOpen2 = makeAsyncTest(bait2URI, testHeaderVal2, testViaAsyncOpen3);
+  testViaAsyncOpen  = makeAsyncTest(baitURI,  testHeaderVal, testViaAsyncOpen2);
+
+  testViaXHR();
+  testViaAsyncOpen();  // will call done() asynchronously for cleanup
+
+  do_test_pending();
+}

netwerk/test/unit/xpcshell.ini

@@ -263,6 +263,7 @@ fail-if = os == "android"
 # Bug 675039: test fails consistently on Android
 fail-if = os == "android"
 [test_redirect_from_script.js]
+[test_redirect_from_script_after-open_passing.js]
 [test_redirect_passing.js]
 [test_redirect_loop.js]
 [test_redirect_baduri.js]