Bug 704539 - Better handling of HTTP channels in Sync; r=rnewman

services/sync/modules/resource.js

@@ -244,8 +244,8 @@ AsyncResource.prototype = {
     channel.loadFlags |= Ci.nsIRequest.LOAD_BYPASS_CACHE;
     channel.loadFlags |= Ci.nsIRequest.INHIBIT_CACHING;
 
-    // Setup a callback to handle bad HTTPS certificates.
-    channel.notificationCallbacks = new BadCertListener();
+    // Setup a callback to handle channel notifications.
+    channel.notificationCallbacks = new ChannelNotificationListener();
 
     // Compose a UA string fragment from the various available identifiers.
     if (Svc.Prefs.get("sendVersionInfo", true)) {
@@ -520,7 +520,14 @@ ChannelListener.prototype = {
 
   onStartRequest: function Channel_onStartRequest(channel) {
     this._log.trace("onStartRequest called for channel " + channel + ".");
-    channel.QueryInterface(Ci.nsIHttpChannel);
+
+    try {
+      channel.QueryInterface(Ci.nsIHttpChannel);
+    } catch (ex) {
+      this._log.error("Unexpected error: channel is not a nsIHttpChannel!");
+      channel.cancel(Cr.NS_BINDING_ABORTED);
+      return;
+    }
 
     // Save the latest server timestamp when possible.
     try {
@@ -538,6 +545,22 @@ ChannelListener.prototype = {
     // Clear the abort timer now that the channel is done.
     this.abortTimer.clear();
 
+    if (!this._onComplete) {
+      this._log.error("Unexpected error: _onComplete not defined in onStopRequest.");
+      this._onProgress = null;
+      return;
+    }
+
+    try {
+      channel.QueryInterface(Ci.nsIHttpChannel);
+    } catch (ex) {
+      this._log.error("Unexpected error: channel is not a nsIHttpChannel!");
+
+      this._onComplete(ex, this._data, channel);
+      this._onComplete = this._onProgress = null;
+      return;
+    }
+
     let statusSuccess = Components.isSuccessCode(status);
     let uri = channel && channel.URI && channel.URI.spec || "<unknown>";
     this._log.trace("Channel for " + channel.requestMethod + " " + uri + ": " +
@@ -553,7 +576,9 @@ ChannelListener.prototype = {
     if (!statusSuccess) {
       let message = Components.Exception("", status).name;
       let error   = Components.Exception(message, status);
+
       this._onComplete(error, undefined, channel);
+      this._onComplete = this._onProgress = null;
       return;
     }
 
@@ -561,6 +586,7 @@ ChannelListener.prototype = {
                     ", URI = " + uri +
                     ", HTTP success? " + channel.requestSucceeded);
     this._onComplete(null, this._data, channel);
+    this._onComplete = this._onProgress = null;
   },
 
   onDataAvailable: function Channel_onDataAvail(req, cb, stream, off, count) {
@@ -600,40 +626,49 @@ ChannelListener.prototype = {
     this.onStopRequest = function() {};
     let error = Components.Exception("Aborting due to channel inactivity.",
                                      Cr.NS_ERROR_NET_TIMEOUT);
+    if (!this._onComplete) {
+      this._log.error("Unexpected error: _onComplete not defined in " +
+                      "abortRequest.");
+      return;
+    }
     this._onComplete(error);
   }
 };
 
-// = BadCertListener =
-//
-// We use this listener to ignore bad HTTPS
-// certificates and continue a request on a network
-// channel. Probably not a very smart thing to do,
-// but greatly simplifies debugging and is just very
-// convenient.
-function BadCertListener() {
+/**
+ * This class handles channel notification events.
+ *
+ * An instance of this class is bound to each created channel.
+ */
+function ChannelNotificationListener() {
 }
-BadCertListener.prototype = {
+ChannelNotificationListener.prototype = {
   getInterface: function(aIID) {
     return this.QueryInterface(aIID);
   },
 
   QueryInterface: function(aIID) {
-    if (aIID.equals(Components.interfaces.nsIBadCertListener2) ||
-        aIID.equals(Components.interfaces.nsIInterfaceRequestor) ||
-        aIID.equals(Components.interfaces.nsISupports))
+    if (aIID.equals(Ci.nsIBadCertListener2) ||
+        aIID.equals(Ci.nsIInterfaceRequestor) ||
+        aIID.equals(Ci.nsISupports) ||
+        aIID.equals(Ci.nsIChannelEventSink))
       return this;
 
-    throw Components.results.NS_ERROR_NO_INTERFACE;
+    throw Cr.NS_ERROR_NO_INTERFACE;
   },
 
   notifyCertProblem: function certProblem(socketInfo, sslStatus, targetHost) {
-    // Silently ignore?
     let log = Log4Moz.repository.getLogger("Sync.CertListener");
-    log.level =
-      Log4Moz.Level[Svc.Prefs.get("log.logger.network.resources")];
-    log.debug("Invalid HTTPS certificate encountered, ignoring!");
+    log.warn("Invalid HTTPS certificate encountered!");
 
+    // This suppresses the UI warning only. The request is still cancelled.
     return true;
+  },
+
+  asyncOnChannelRedirect:
+    function asyncOnChannelRedirect(oldChannel, newChannel, flags, callback) {
+
+    // We let all redirects proceed.
+    callback.onRedirectVerifyCallback(Cr.NS_OK);
   }
 };

services/sync/modules/rest.js

@@ -126,7 +126,8 @@ RESTRequest.prototype = {
 
   QueryInterface: XPCOMUtils.generateQI([
     Ci.nsIBadCertListener2,
-    Ci.nsIInterfaceRequestor
+    Ci.nsIInterfaceRequestor,
+    Ci.nsIChannelEventSink
   ]),
 
   /*** Public API: ***/
@@ -364,22 +365,33 @@ RESTRequest.prototype = {
     this.abort();
     let error = Components.Exception("Aborting due to channel inactivity.",
                                      Cr.NS_ERROR_NET_TIMEOUT);
+    if (!this.onComplete) {
+      this._log.error("Unexpected error: onComplete not defined in " +
+                      "abortTimeout.")
+      return;
+    }
     this.onComplete(error);
   },
 
   /*** nsIStreamListener ***/
 
   onStartRequest: function onStartRequest(channel) {
-    // Update the channel in case we got redirected.
-    this.channel = channel;
-
     if (this.status == this.ABORTED) {
       this._log.trace("Not proceeding with onStartRequest, request was aborted.");
       return;
     }
+
+    try {
+      channel.QueryInterface(Ci.nsIHttpChannel);
+    } catch (ex) {
+      this._log.error("Unexpected error: channel is not a nsIHttpChannel!");
+      this.status = this.ABORTED;
+      channel.cancel(Cr.NS_BINDING_ABORTED);
+      return;
+    }
+
     this.status = this.IN_PROGRESS;
 
-    channel.QueryInterface(Ci.nsIHttpChannel);
     this._log.trace("onStartRequest: " + channel.requestMethod + " " +
                     channel.URI.spec);
 
@@ -397,9 +409,6 @@ RESTRequest.prototype = {
   },
 
   onStopRequest: function onStopRequest(channel, context, statusCode) {
-    // Update the channel in case we got redirected.
-    this.channel = channel;
-
     if (this.timeoutTimer) {
       // Clear the abort timer now that the channel is done.
       this.timeoutTimer.clear();
@@ -410,6 +419,14 @@ RESTRequest.prototype = {
       this._log.trace("Not proceeding with onStopRequest, request was aborted.");
       return;
     }
+
+    try {
+      channel.QueryInterface(Ci.nsIHttpChannel);
+    } catch (ex) {
+      this._log.error("Unexpected error: channel not nsIHttpChannel!");
+      this.status = this.ABORTED;
+      return;
+    }
     this.status = this.COMPLETED;
 
     let statusSuccess = Components.isSuccessCode(statusCode);
@@ -417,6 +434,13 @@ RESTRequest.prototype = {
     this._log.trace("Channel for " + channel.requestMethod + " " + uri +
                     " returned status code " + statusCode);
 
+    if (!this.onComplete) {
+      this._log.error("Unexpected error: onComplete not defined in " +
+                      "abortRequest.");
+      this.onProgress = null;
+      return;
+    }
+
     // Throw the failure code and stop execution.  Use Components.Exception()
     // instead of Error() so the exception is QI-able and can be passed across
     // XPCOM borders while preserving the status code.
@@ -459,6 +483,14 @@ RESTRequest.prototype = {
                      this.method + " " + req.URI.spec);
       this._log.debug("Exception: " + Utils.exceptionStr(ex));
       this.abort();
+
+      if (!this.onComplete) {
+        this._log.error("Unexpected error: onComplete not defined in " +
+                        "onDataAvailable.");
+        this.onProgress = null;
+        return;
+      }
+
       this.onComplete(ex);
       this.onComplete = this.onProgress = null;
       return;
@@ -480,6 +512,24 @@ RESTRequest.prototype = {
     // Suppress invalid HTTPS certificate warnings in the UI.
     // (The request will still fail.)
     return true;
+  },
+
+  /*** nsIChannelEventSink ***/
+  asyncOnChannelRedirect:
+    function asyncOnChannelRedirect(oldChannel, newChannel, flags, callback) {
+
+    try {
+      newChannel.QueryInterface(Ci.nsIHttpChannel);
+    } catch (ex) {
+      this._log.error("Unexpected error: channel not nsIHttpChannel!");
+      callback.onRedirectVerifyCallback(Cr.NS_ERROR_NO_INTERFACE);
+      return;
+    }
+
+    this.channel = newChannel;
+
+    // We let all redirects proceed.
+    callback.onRedirectVerifyCallback(Cr.NS_OK);
   }
 };
 

services/sync/tests/unit/head_http_server.js

@@ -23,9 +23,9 @@ function return_timestamp(request, response, timestamp) {
   return timestamp;
 }
 
-function httpd_setup (handlers) {
+function httpd_setup (handlers, port) {
+  let port   = port || 8080;
   let server = new nsHttpServer();
-  let port   = 8080;
   for (let path in handlers) {
     server.registerPathHandler(path, handlers[path]);
   }

services/sync/tests/unit/test_resource_async.js

@@ -145,6 +145,13 @@ function server_headers(metadata, response) {
   response.bodyOutputStream.write(body, body.length);
 }
 
+function server_redirect(metadata, response) {
+  let body = "Redirecting";
+  response.setStatusLine(metadata.httpVersion, 307, "TEMPORARY REDIRECT");
+  response.setHeader("Location", "http://localhost:8081/resource");
+  response.bodyOutputStream.write(body, body.length);
+}
+
 let quotaValue;
 Observers.add("weave:service:quota:remaining",
               function (subject) { quotaValue = subject; });
@@ -167,7 +174,8 @@ function run_test() {
     "/backoff": server_backoff,
     "/pac2": server_pac,
     "/quota-notice": server_quota_notice,
-    "/quota-error": server_quota_error
+    "/quota-error": server_quota_error,
+    "/redirect": server_redirect
   });
 
   Svc.Prefs.set("network.numRetries", 1); // speed up test
@@ -658,6 +666,31 @@ add_test(function test_uri_construction() {
   run_next_test();
 });
 
+add_test(function test_new_channel() {
+  _("Ensure a redirect to a new channel is handled properly.");
+
+  let resourceRequested = false;
+  function resourceHandler(metadata, response) {
+    resourceRequested = true;
+
+    let body = "Test";
+    response.setHeader("Content-Type", "text/plain");
+    response.bodyOutputStream.write(body, body.length);
+  }
+  let server2 = httpd_setup({"/resource": resourceHandler}, 8081);
+
+  let request = new AsyncResource("http://localhost:8080/redirect");
+  request.get(function onRequest(error, content) {
+    do_check_null(error);
+    do_check_true(resourceRequested);
+    do_check_eq(200, content.status);
+    do_check_true("content-type" in content.headers);
+    do_check_eq("text/plain", content.headers["content-type"]);
+
+    server2.stop(run_next_test);
+  });
+});
+
 add_test(function tear_down() {
   server.stop(run_next_test);
 });

services/sync/tests/unit/test_restrequest.js

@@ -620,3 +620,44 @@ add_test(function test_exception_in_onProgress() {
     server.stop(run_next_test);
   });
 });
+
+add_test(function test_new_channel() {
+  _("Ensure a redirect to a new channel is handled properly.");
+
+  let redirectRequested = false;
+  function redirectHandler(metadata, response) {
+    redirectRequested = true;
+
+    let body = "Redirecting";
+    response.setStatusLine(metadata.httpVersion, 307, "TEMPORARY REDIRECT");
+    response.setHeader("Location", "http://localhost:8081/resource");
+    response.bodyOutputStream.write(body, body.length);
+  }
+
+  let resourceRequested = false;
+  function resourceHandler(metadata, response) {
+    resourceRequested = true;
+
+    let body = "Test";
+    response.setHeader("Content-Type", "text/plain");
+    response.bodyOutputStream.write(body, body.length);
+  }
+  let server1 = httpd_setup({"/redirect": redirectHandler}, 8080);
+  let server2 = httpd_setup({"/resource": resourceHandler}, 8081);
+
+  function advance() {
+    server1.stop(function () {
+      server2.stop(run_next_test);
+    });
+  }
+
+  let request = new RESTRequest("http://localhost:8080/redirect");
+  request.get(function onComplete(error) {
+    let response = this.response;
+
+    do_check_eq(200, response.status);
+    do_check_eq("Test", response.body);
+
+    advance();
+  });
+});