Bug 1032414: Always return failure in OnStopRequest on network error (r=gcp)
authorMonica Chew <mmc@mozilla.com>
Fri, 07 Nov 2014 07:12:37 -0800
changeset 220054 89c76328ade6f016dcc86591fcdfd43fca8c9aeb
parent 220052 df0850b9023b6dfaead4817cfc3e42902a8ec496
child 220055 e2c20f36abffd41e5038341b486b0d91e96fc5c8
push id27975
push usercbook@mozilla.com
push dateWed, 17 Dec 2014 11:46:23 +0000
treeherdermozilla-central@494f71e59583 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersgcp
bugs1032414
milestone37.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1032414: Always return failure in OnStopRequest on network error (r=gcp)
toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.cpp
toolkit/components/url-classifier/tests/unit/head_urlclassifier.js
toolkit/components/url-classifier/tests/unit/test_streamupdater.js
--- a/toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.cpp
+++ b/toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.cpp
@@ -335,24 +335,29 @@ nsUrlClassifierStreamUpdater::FetchNextR
 
   return NS_OK;
 }
 
 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)) {
     rv = mTimer->InitWithCallback(this, requestedDelay,
                                   nsITimer::TYPE_ONE_SHOT);
   }
 
   if (NS_FAILED(rv)) {
@@ -373,17 +378,22 @@ nsUrlClassifierStreamUpdater::UpdateSucc
 
   // DownloadDone() clears mSuccessCallback, so we save it off here.
   nsCOMPtr<nsIUrlClassifierCallback> successCallback = mDownloadError ? nullptr : mSuccessCallback.get();
   DownloadDone();
 
   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"));
   FetchNextRequest();
 
   return NS_OK;
 }
 
@@ -447,50 +457,53 @@ nsUrlClassifierStreamUpdater::OnStartReq
   bool downloadError = false;
   nsAutoCString strStatus;
   nsresult status = NS_OK;
 
   // Only update if we got http success header
   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 ||
-        NS_ERROR_NET_TIMEOUT == status) {
+    if (NS_FAILED(status)) {
       // Assume we're overloading the server and trigger backoff.
       downloadError = true;
-    }
-
-    if (NS_SUCCEEDED(status)) {
+    } else {
       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."));
 
         uint32_t requestStatus;
         rv = httpChannel->GetResponseStatus(&requestStatus);
         LOG(("HTTP request returned failure code: %d.", requestStatus));
         NS_ENSURE_SUCCESS(rv, rv);
 
         strStatus.AppendInt(requestStatus);
         downloadError = true;
       }
     }
   }
 
   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);
   }
 
   mStreamTable.Truncate();
 
   return status;
 }
@@ -523,37 +536,45 @@ nsUrlClassifierStreamUpdater::OnDataAvai
 
 NS_IMETHODIMP
 nsUrlClassifierStreamUpdater::OnStopRequest(nsIRequest *request, nsISupports* context,
                                             nsresult aStatus)
 {
   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;
 
   if (NS_SUCCEEDED(aStatus)) {
     // 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.
     rv = mDBService->FinishUpdate();
   }
 
   mChannel = nullptr;
 
-  return rv;
+  // 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
 
 NS_IMETHODIMP
 nsUrlClassifierStreamUpdater::Observe(nsISupports *aSubject, const char *aTopic,
                                       const char16_t *aData)
--- a/toolkit/components/url-classifier/tests/unit/head_urlclassifier.js
+++ b/toolkit/components/url-classifier/tests/unit/head_urlclassifier.js
@@ -171,18 +171,19 @@ function doErrorUpdate(tables, success, 
 
 /**
  * Performs an update of the dbservice using the stream updater and a
  * data: uri
  */
 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);
 }
 
 var gAssertions = {
 
 tableData : function(expectedTables, cb)
--- a/toolkit/components/url-classifier/tests/unit/test_streamupdater.js
+++ b/toolkit/components/url-classifier/tests/unit/test_streamupdater.js
@@ -2,16 +2,18 @@ function doTest(updates, assertions, exp
 {
   if (expectError) {
     doUpdateTest(updates, assertions, updateError, runNextTest);
   } else {
     doUpdateTest(updates, assertions, runNextTest, updateError);
   }
 }
 
+// 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" ];
 
   var update = "n:1000\n";
   update += "i:test-phish-simple\n";
 
   var update1 = buildBareUpdate(
     [{ "chunkNum" : 1,
@@ -22,19 +24,19 @@ function testFillDb() {
     "tableData" : "test-phish-simple;a:1",
     "urlsExist" : add1Urls
   };
 
   doTest([update], assertions, false);
 }
 
 function testSimpleForward() {
-  var add1Urls = [ "foo.com/a", "bar.com/c" ];
-  var add2Urls = [ "foo.com/b" ];
-  var add3Urls = [ "bar.com/d" ];
+  var add1Urls = [ "foo-simple.com/a", "bar-simple.com/c" ];
+  var add2Urls = [ "foo-simple.com/b" ];
+  var add3Urls = [ "bar-simple.com/d" ];
 
   var update = "n:1000\n";
   update += "i:test-phish-simple\n";
 
   var update1 = buildBareUpdate(
     [{ "chunkNum" : 1,
        "urls" : add1Urls }]);
   update += "u:data:," + encodeURIComponent(update1) + "\n";
@@ -55,18 +57,18 @@ function testSimpleForward() {
   };
 
   doTest([update], assertions, false);
 }
 
 // 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 add2Urls = [ "foo.com/b" ];
+  var add1Urls = [ "foo-nested.com/a", "bar-nested.com/c" ];
+  var add2Urls = [ "foo-nested.com/b" ];
 
   var update = "n:1000\n";
   update += "i:test-phish-simple\n";
 
   var update1 = buildBareUpdate(
     [{ "chunkNum" : 1,
        "urls" : add1Urls }]);
   update += "u:data:," + encodeURIComponent(update1) + "\n";
@@ -86,56 +88,54 @@ function testNestedForward() {
     "urlsDontExist" : add1Urls.concat(add2Urls)
   };
 
   doTest([update], assertions, true);
 }
 
 // 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 add2Urls = [ "foo.com/b" ];
-  var add3Urls = [ "bar.com/d" ];
+  var add1Urls = [ "foo-multiple.com/a", "bar-multiple.com/c" ];
+  var add2Urls = [ "foo-multiple.com/b" ];
+  var add3Urls = [ "bar-multiple.com/d" ];
 
   var update = "n:1000\n";
   update += "i:test-phish-simple\n";
 
   var update1 = buildBareUpdate(
     [{ "chunkNum" : 1,
        "urls" : add1Urls }]);
   update += "u:data:," + encodeURIComponent(update1) + "\n";
@@ -174,26 +174,26 @@ QueryInterface: function(iid)
     throw Cr.NS_ERROR_NO_INTERFACE;
   }
   return this;
 }
 };
 
 // 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,
         "urls" : addUrls1
       }]);
 
   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,
         "urls" : addUrls3
       }]);
 
   var assertions = {
     "tableData" : "test-phish-simple;a:3",