Bug 1346757 - Change the downloadError callback timing. draft
authorHenry Chang <hchang@mozilla.com>
Mon, 13 Mar 2017 21:16:07 +0800
changeset 497492 e30cc47e5590fc396f887ad0a7c09ce7b9d5e4ed
parent 493721 8d026c60151005ad942e3d4389318fe28a0c8c54
child 548916 e8e45aaed1cab6c5522d8f92dfbd4a091c093d5e
push id48929
push userhchang@mozilla.com
push dateMon, 13 Mar 2017 13:19:40 +0000
bugs1346757
milestone54.0a1
Bug 1346757 - Change the downloadError callback timing. MozReview-Commit-ID: JleLPltEBOw
toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.cpp
toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.h
toolkit/components/url-classifier/tests/unit/test_streamupdater.js
--- a/toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.cpp
+++ b/toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.cpp
@@ -352,17 +352,16 @@ nsUrlClassifierStreamUpdater::FetchNext(
   nsresult rv = FetchUpdate(update.mUrl,
                             EmptyCString(),
                             true, // This method is for v2 and v2 is always a POST.
                             update.mTable);
   if (NS_FAILED(rv)) {
     LOG(("Error fetching update url: %s\n", update.mUrl.get()));
     // We can commit the urls that we've applied so far.  This is
     // probably a transient server problem, so trigger backoff.
-    mDownloadErrorCallback->HandleEvent(EmptyCString());
     mDownloadError = true;
     mDBService->FinishUpdate();
     return rv;
   }
 
   mPendingUpdates.RemoveElementAt(0);
 
   return NS_OK;
@@ -437,49 +436,55 @@ nsUrlClassifierStreamUpdater::UpdateSucc
 {
   LOG(("nsUrlClassifierStreamUpdater::UpdateSuccess [this=%p]", this));
   if (mPendingUpdates.Length() != 0) {
     NS_WARNING("Didn't fetch all safebrowsing update redirects");
   }
 
   // DownloadDone() clears mSuccessCallback, so we save it off here.
   nsCOMPtr<nsIUrlClassifierCallback> successCallback = mDownloadError ? nullptr : mSuccessCallback.get();
+  nsCOMPtr<nsIUrlClassifierCallback> downloadErrorCallback = mDownloadError ? mDownloadErrorCallback.get() : nullptr;
   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));
+  } else if (downloadErrorCallback) {
+    downloadErrorCallback->HandleEvent(mDownloadErrorStatusStr);
+    mDownloadErrorStatusStr = EmptyCString();
+    LOG(("Notify download error callback in UpdateSuccess [this=%p]", this));
   }
   // Now fetch the next request
   LOG(("stream updater: calling into fetch next request"));
   FetchNextRequest();
 
   return NS_OK;
 }
 
 NS_IMETHODIMP
 nsUrlClassifierStreamUpdater::UpdateError(nsresult result)
 {
   LOG(("nsUrlClassifierStreamUpdater::UpdateError [this=%p]", this));
 
   // DownloadDone() clears mUpdateErrorCallback, so we save it off here.
   nsCOMPtr<nsIUrlClassifierCallback> errorCallback = mDownloadError ? nullptr : mUpdateErrorCallback.get();
-
+  nsCOMPtr<nsIUrlClassifierCallback> downloadErrorCallback = mDownloadError ? mDownloadErrorCallback.get() : nullptr;
   DownloadDone();
 
   nsAutoCString strResult;
   strResult.AppendInt(static_cast<uint32_t>(result));
   if (errorCallback) {
     errorCallback->HandleEvent(strResult);
+  } else if (downloadErrorCallback) {
+    LOG(("Notify download error callback in UpdateError [this=%p]", this));
+    downloadErrorCallback->HandleEvent(mDownloadErrorStatusStr);
+    mDownloadErrorStatusStr = EmptyCString();
   }
 
   return NS_OK;
 }
 
 nsresult
 nsUrlClassifierStreamUpdater::AddRequestBody(const nsACString &aRequestBody)
 {
@@ -700,23 +705,18 @@ nsUrlClassifierStreamUpdater::OnStartReq
         strStatus.AppendInt(requestStatus);
         downloadError = true;
       }
     }
   }
 
   if (downloadError) {
     LOG(("nsUrlClassifierStreamUpdater::Download error [this=%p]", this));
-
-    // It's possible for mDownloadErrorCallback to be null on shutdown.
-    if (mDownloadErrorCallback) {
-      mDownloadErrorCallback->HandleEvent(strStatus);
-    }
-
     mDownloadError = true;
+    mDownloadErrorStatusStr = strStatus;
     status = NS_ERROR_ABORT;
   } else if (NS_SUCCEEDED(status)) {
     MOZ_ASSERT(mDownloadErrorCallback);
     mBeganStream = true;
     LOG(("nsUrlClassifierStreamUpdater::Beginning stream [this=%p]", this));
     rv = mDBService->BeginStream(mStreamTable);
     NS_ENSURE_SUCCESS(rv, rv);
   }
--- a/toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.h
+++ b/toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.h
@@ -69,16 +69,18 @@ private:
   nsresult FetchNextRequest();
 
 
   bool mIsUpdating;
   bool mInitialized;
   bool mDownloadError;
   bool mBeganStream;
 
+  nsCString mDownloadErrorStatusStr;
+
   // Note that mStreamTable is only used by v2, it is empty for v4 update.
   nsCString mStreamTable;
 
   nsCOMPtr<nsIChannel> mChannel;
   nsCOMPtr<nsIUrlClassifierDBService> mDBService;
   nsCOMPtr<nsITimer> mTimer;
 
   struct PendingRequest {
--- a/toolkit/components/url-classifier/tests/unit/test_streamupdater.js
+++ b/toolkit/components/url-classifier/tests/unit/test_streamupdater.js
@@ -97,17 +97,17 @@ function testInvalidUrlForward() {
 
   var update = buildPhishingUpdate(
     [{ "chunkNum" : 1,
        "urls" : add1Urls }]);
   update += "u:asdf://blah/blah\n";  // invalid URL scheme
 
   // add1Urls is present, but that is an artifact of the way we do the test.
   var assertions = {
-    "tableData" : "",
+    "tableData" : "test-phish-simple;a:1",
     "urlsExist" : add1Urls
   };
 
   doTest([update], assertions, true);
 }
 
 // A failed network request causes the update to fail.
 function testErrorUrlForward() {
@@ -115,17 +115,17 @@ function testErrorUrlForward() {
 
   var update = buildPhishingUpdate(
     [{ "chunkNum" : 1,
        "urls" : add1Urls }]);
   update += "u:http://test.invalid/asdf/asdf\n";  // invalid URL scheme
 
   // add1Urls is present, but that is an artifact of the way we do the test.
   var assertions = {
-    "tableData" : "",
+    "tableData" : "test-phish-simple;a:1",
     "urlsExist" : add1Urls
   };
 
   doTest([update], assertions, true);
 }
 
 function testMultipleTables() {
   var add1Urls = [ "foo-multiple.com/a", "bar-multiple.com/c" ];