Bug 1032414: Always return failure in OnStopRequest on network error (r=gcp)

toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.cpp

@@ -340,14 +340,19 @@ NS_IMETHODIMP
 nsUrlClassifierStreamUpdater::StreamFinished(nsresult status,
                                              uint32_t requestedDelay)
 {
+  // We are a service and may not be reset with Init between calls, so reset
+  // mBeganStream manually.
+  mBeganStream = false;
   LOG(("nsUrlClassifierStreamUpdater::StreamFinished [%x, %d]", status, requestedDelay));
   if (NS_FAILED(status) || mPendingUpdates.Length() == 0) {
     // We're done.
+    LOG(("nsUrlClassifierStreamUpdater::Done [this=%p]", this));
     mDBService->FinishUpdate();
     return NS_OK;
   }
 
   // Wait the requested amount of time before starting a new stream.
+  // This appears to be a duplicate timer (see bug 1110891)
   nsresult rv;
   mTimer = do_CreateInstance("@mozilla.org/timer;1", &rv);
   if (NS_SUCCEEDED(rv)) {
@@ -378,7 +383,12 @@ nsUrlClassifierStreamUpdater::UpdateSuccess(uint32_t requestedTimeout)
   nsAutoCString strTimeout;
   strTimeout.AppendInt(requestedTimeout);
   if (successCallback) {
+    LOG(("nsUrlClassifierStreamUpdater::UpdateSuccess callback [this=%p]",
+         this));
     successCallback->HandleEvent(strTimeout);
+  } else {
+    LOG(("nsUrlClassifierStreamUpdater::UpdateSuccess skipping callback [this=%p]",
+         this));
   }
   // Now fetch the next request
   LOG(("stream updater: calling into fetch next request"));
@@ -452,19 +462,20 @@ nsUrlClassifierStreamUpdater::OnStartRequest(nsIRequest *request,
   nsCOMPtr<nsIHttpChannel> httpChannel = do_QueryInterface(request);
   if (httpChannel) {
     rv = httpChannel->GetStatus(&status);
+    LOG(("nsUrlClassifierStreamUpdater::OnStartRequest (status=%x, this=%p)",
+         status, this));
     NS_ENSURE_SUCCESS(rv, rv);
 
-    if (NS_ERROR_CONNECTION_REFUSED == status ||
+    if (NS_FAILED(status)) {
-        NS_ERROR_NET_TIMEOUT == status) {
       // Assume we're overloading the server and trigger backoff.
       downloadError = true;
-    }
+    } else {
-
-    if (NS_SUCCEEDED(status)) {
       bool succeeded = false;
       rv = httpChannel->GetRequestSucceeded(&succeeded);
       NS_ENSURE_SUCCESS(rv, rv);
 
+      LOG(("nsUrlClassifierStreamUpdater::OnStartRequest (%s)", succeeded ?
+           "succeeded" : "failed"));
       if (!succeeded) {
         // 404 or other error, pass error status back
         LOG(("HTTP request returned failure code."));
@@ -481,11 +492,13 @@ nsUrlClassifierStreamUpdater::OnStartRequest(nsIRequest *request,
   }
 
   if (downloadError) {
+    LOG(("nsUrlClassifierStreamUpdater::Download error [this=%p]", this));
     mDownloadErrorCallback->HandleEvent(strStatus);
     mDownloadError = true;
     status = NS_ERROR_ABORT;
   } else if (NS_SUCCEEDED(status)) {
     mBeganStream = true;
+    LOG(("nsUrlClassifierStreamUpdater::Beginning stream [this=%p]", this));
     rv = mDBService->BeginStream(mStreamTable);
     NS_ENSURE_SUCCESS(rv, rv);
   }
@@ -528,7 +541,8 @@ nsUrlClassifierStreamUpdater::OnStopRequest(nsIRequest *request, nsISupports* co
   if (!mDBService)
     return NS_ERROR_NOT_INITIALIZED;
 
-  LOG(("OnStopRequest (status %x)", aStatus));
+  LOG(("OnStopRequest (status %x, beganStream %s, this=%p)", aStatus,
+       mBeganStream ? "true" : "false", this));
 
   nsresult rv;
 
@@ -536,10 +550,12 @@ nsUrlClassifierStreamUpdater::OnStopRequest(nsIRequest *request, nsISupports* co
     // Success, finish this stream and move on to the next.
     rv = mDBService->FinishStream();
   } else if (mBeganStream) {
+    LOG(("OnStopRequest::Canceling update [this=%p]", this));
     // We began this stream and couldn't finish it.  We have to cancel the
     // update, it's not in a consistent state.
     rv = mDBService->CancelUpdate();
   } else {
+    LOG(("OnStopRequest::Finishing update [this=%p]", this));
     // The fetch failed, but we didn't start the stream (probably a
     // server or connection error).  We can commit what we've applied
     // so far, and request again later.
@@ -548,8 +564,13 @@ nsUrlClassifierStreamUpdater::OnStopRequest(nsIRequest *request, nsISupports* co
 
   mChannel = nullptr;
 
+  // If the fetch failed, return the network status rather than NS_OK, the
+  // result of finishing a possibly-empty update
+  if (NS_SUCCEEDED(aStatus)) {
     return rv;
   }
+  return aStatus;
+}
 
 ///////////////////////////////////////////////////////////////////////////////
 // nsIObserver implementation

toolkit/components/url-classifier/tests/unit/head_urlclassifier.js

@@ -176,8 +176,9 @@ function doErrorUpdate(tables, success, failure) {
 function doStreamUpdate(updateText, success, failure, downloadFailure) {
   var dataUpdate = "data:," + encodeURIComponent(updateText);
 
-  if (!downloadFailure)
+  if (!downloadFailure) {
     downloadFailure = failure;
+  }
 
   streamUpdater.downloadUpdates("test-phish-simple,test-malware-simple", "",
                                 dataUpdate, success, failure, downloadFailure);

toolkit/components/url-classifier/tests/unit/test_streamupdater.js

@@ -7,6 +7,8 @@ function doTest(updates, assertions, expectError)
   }
 }
 
+// Never use the same URLs for multiple tests, because we aren't guaranteed
+// to reset the database between tests.
 function testFillDb() {
   var add1Urls = [ "zaz.com/a", "yxz.com/c" ];
 
@@ -27,9 +29,9 @@ function testFillDb() {
 }
 
 function testSimpleForward() {
-  var add1Urls = [ "foo.com/a", "bar.com/c" ];
+  var add1Urls = [ "foo-simple.com/a", "bar-simple.com/c" ];
-  var add2Urls = [ "foo.com/b" ];
+  var add2Urls = [ "foo-simple.com/b" ];
-  var add3Urls = [ "bar.com/d" ];
+  var add3Urls = [ "bar-simple.com/d" ];
 
   var update = "n:1000\n";
   update += "i:test-phish-simple\n";
@@ -60,8 +62,8 @@ function testSimpleForward() {
 // Make sure that a nested forward (a forward within a forward) causes
 // the update to fail.
 function testNestedForward() {
-  var add1Urls = [ "foo.com/a", "bar.com/c" ];
+  var add1Urls = [ "foo-nested.com/a", "bar-nested.com/c" ];
-  var add2Urls = [ "foo.com/b" ];
+  var add2Urls = [ "foo-nested.com/b" ];
 
   var update = "n:1000\n";
   update += "i:test-phish-simple\n";
@@ -91,46 +93,44 @@ function testNestedForward() {
 
 // An invalid URL forward causes the update to fail.
 function testInvalidUrlForward() {
-  var add1Urls = [ "foo.com/a", "bar.com/c" ];
+  var add1Urls = [ "foo-invalid.com/a", "bar-invalid.com/c" ];
 
   var update = buildPhishingUpdate(
     [{ "chunkNum" : 1,
        "urls" : add1Urls }]);
   update += "u:asdf://blah/blah\n";  // invalid URL scheme
 
-  // The first part of the update should have succeeded.
+  // add1Urls is present, but that is an artifact of the way we do the test.
-
   var assertions = {
-    "tableData" : "test-phish-simple;a:1",
+    "tableData" : "",
     "urlsExist" : add1Urls
   };
 
-  doTest([update], assertions, false);
+  doTest([update], assertions, true);
 }
 
 // A failed network request causes the update to fail.
 function testErrorUrlForward() {
-  var add1Urls = [ "foo.com/a", "bar.com/c" ];
+  var add1Urls = [ "foo-forward.com/a", "bar-forward.com/c" ];
 
   var update = buildPhishingUpdate(
     [{ "chunkNum" : 1,
        "urls" : add1Urls }]);
   update += "u:http://test.invalid/asdf/asdf\n";  // invalid URL scheme
 
-  // The first part of the update should have succeeded
+  // add1Urls is present, but that is an artifact of the way we do the test.
-
   var assertions = {
-    "tableData" : "test-phish-simple;a:1",
+    "tableData" : "",
     "urlsExist" : add1Urls
   };
 
-  doTest([update], assertions, false);
+  doTest([update], assertions, true);
 }
 
 function testMultipleTables() {
-  var add1Urls = [ "foo.com/a", "bar.com/c" ];
+  var add1Urls = [ "foo-multiple.com/a", "bar-multiple.com/c" ];
-  var add2Urls = [ "foo.com/b" ];
+  var add2Urls = [ "foo-multiple.com/b" ];
-  var add3Urls = [ "bar.com/d" ];
+  var add3Urls = [ "bar-multiple.com/d" ];
 
   var update = "n:1000\n";
   update += "i:test-phish-simple\n";
@@ -179,7 +179,7 @@ QueryInterface: function(iid)
 
 // Tests a database reset request.
 function testReset() {
-  var addUrls1 = [ "foo.com/a", "foo.com/b" ];
+  var addUrls1 = [ "foo-reset.com/a", "foo-reset.com/b" ];
   var update1 = buildPhishingUpdate(
     [
       { "chunkNum" : 1,
@@ -188,7 +188,7 @@ function testReset() {
 
   var update2 = "n:1000\nr:pleasereset\n";
 
-  var addUrls3 = [ "bar.com/a", "bar.com/b" ];
+  var addUrls3 = [ "bar-reset.com/a", "bar-reset.com/b" ];
   var update3 = buildPhishingUpdate(
     [
       { "chunkNum" : 3,