Bug 1338638 - Fix race condition for DBService APIs to avoid long delayed initial download. draft
authorHenry Chang <hchang@mozilla.com>
Tue, 11 Apr 2017 17:13:01 +0800
changeset 561052 6a657fcc7401dafe6f646738748a516e9ef26e4f
parent 560309 c05000863d49dd34483a779725de39a47deedbe1
child 623865 b9299e31634c53e5c23c2cad15a9033a42539372
push id53610
push userhchang@mozilla.com
push dateWed, 12 Apr 2017 05:47:48 +0000
bugs1338638
milestone55.0a1
Bug 1338638 - Fix race condition for DBService APIs to avoid long delayed initial download. When starting up, SafeBrowsing.jsm will try to use DBService to add testing entries. Meanwhile, listmanager will request StreamUpdater to download lists with a random initial delay. The requests that listmanager issue to StreamUpdater will be queued up if DBserve is busy and will be retried when StreamUpdater is notified that the previous update is complete. However, in some edge cases, the queued requests may not be processed until the next update request from listmanager. For example, SafeBrowsing.jsm calls DBService.beginUpdate at t0 and the update is complete at t1. If listmanager sends all requests via StreamUpdate between t0 and t1, they will all be queued up and no further request can trigger the queued ones. So in this patch I add a timer to re-trigger FetchNextRequest() in case StreamUpdater is not notified the previous update is complete. MozReview-Commit-ID: 3hHsS5N7WRI
toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.cpp
toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.h
--- a/toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.cpp
+++ b/toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.cpp
@@ -26,16 +26,20 @@
 #include "nsIURLFormatter.h"
 #include "Classifier.h"
 
 static const char* gQuitApplicationMessage = "quit-application";
 
 // Limit the list file size to 32mb
 const uint32_t MAX_FILE_SIZE = (32 * 1024 * 1024);
 
+// Retry delay when we failed to DownloadUpdate() if due to
+// DBService busy.
+const uint32_t FETCH_NEXT_REQUEST_RETRY_DELAY_MS = 1000;
+
 #undef LOG
 
 // MOZ_LOG=UrlClassifierStreamUpdater:5
 static mozilla::LazyLogModule gUrlClassifierStreamUpdaterLog("UrlClassifierStreamUpdater");
 #define LOG(args) TrimAndLog args
 
 // Calls nsIURLFormatter::TrimSensitiveURLs to remove sensitive
 // info from the logging message.
@@ -277,16 +281,27 @@ nsUrlClassifierStreamUpdater::DownloadUp
     PendingRequest *request = mPendingRequests.AppendElement();
     request->mTables = aRequestTables;
     request->mRequestPayload = aRequestPayload;
     request->mIsPostRequest = aIsPostRequest;
     request->mUrl = aUpdateUrl;
     request->mSuccessCallback = aSuccessCallback;
     request->mUpdateErrorCallback = aUpdateErrorCallback;
     request->mDownloadErrorCallback = aDownloadErrorCallback;
+
+    // We cannot guarantee that we will be notified when DBService is done
+    // processing the current update, so we fire a retry timer on our own.
+    nsresult rv;
+    mFetchNextRequestTimer = do_CreateInstance("@mozilla.org/timer;1", &rv);
+    if (NS_SUCCEEDED(rv)) {
+      rv = mFetchNextRequestTimer->InitWithCallback(this,
+                                                    FETCH_NEXT_REQUEST_RETRY_DELAY_MS,
+                                                    nsITimer::TYPE_ONE_SHOT);
+    }
+
     return NS_OK;
   }
 
   if (NS_FAILED(rv)) {
     return rv;
   }
 
   nsCOMPtr<nsIUrlClassifierUtils> urlUtil =
@@ -409,20 +424,20 @@ nsUrlClassifierStreamUpdater::StreamFini
     return NS_OK;
   }
 
   // This timer is for fetching indirect updates ("forwards") from any "u:" lines
   // that we encountered while processing the server response. It is NOT for
   // scheduling the next time we pull the list from the server. That's a different
   // timer in listmanager.js (see bug 1110891).
   nsresult rv;
-  mTimer = do_CreateInstance("@mozilla.org/timer;1", &rv);
+  mFetchIndirectUpdatesTimer = do_CreateInstance("@mozilla.org/timer;1", &rv);
   if (NS_SUCCEEDED(rv)) {
-    rv = mTimer->InitWithCallback(this, requestedDelay,
-                                  nsITimer::TYPE_ONE_SHOT);
+    rv = mFetchIndirectUpdatesTimer->InitWithCallback(this, requestedDelay,
+                                                      nsITimer::TYPE_ONE_SHOT);
   }
 
   if (NS_FAILED(rv)) {
     NS_WARNING("Unable to initialize timer, fetching next safebrowsing item immediately");
     return FetchNext();
   }
 
   return NS_OK;
@@ -812,19 +827,23 @@ nsUrlClassifierStreamUpdater::Observe(ns
       LOG(("Cancel download"));
       nsresult rv;
       rv = mChannel->Cancel(NS_ERROR_ABORT);
       NS_ENSURE_SUCCESS(rv, rv);
       mIsUpdating = false;
       mChannel = nullptr;
       mTelemetryClockStart = 0;
     }
-    if (mTimer) {
-      mTimer->Cancel();
-      mTimer = nullptr;
+    if (mFetchIndirectUpdatesTimer) {
+      mFetchIndirectUpdatesTimer->Cancel();
+      mFetchIndirectUpdatesTimer = nullptr;
+    }
+    if (mFetchNextRequestTimer) {
+      mFetchNextRequestTimer->Cancel();
+      mFetchNextRequestTimer = nullptr;
     }
   }
   return NS_OK;
 }
 
 ///////////////////////////////////////////////////////////////////////////////
 // nsIInterfaceRequestor implementation
 
@@ -837,16 +856,25 @@ nsUrlClassifierStreamUpdater::GetInterfa
 
 ///////////////////////////////////////////////////////////////////////////////
 // nsITimerCallback implementation
 NS_IMETHODIMP
 nsUrlClassifierStreamUpdater::Notify(nsITimer *timer)
 {
   LOG(("nsUrlClassifierStreamUpdater::Notify [%p]", this));
 
-  mTimer = nullptr;
+  if (timer == mFetchNextRequestTimer) {
+    mFetchNextRequestTimer = nullptr;
+    FetchNextRequest();
+    return NS_OK;
+  }
 
-  // Start the update process up again.
-  FetchNext();
+  if (timer == mFetchIndirectUpdatesTimer) {
+    mFetchIndirectUpdatesTimer = nullptr;
+    // Start the update process up again.
+    FetchNext();
+    return NS_OK;
+  }
 
+  MOZ_ASSERT_UNREACHABLE("A timer is fired from nowhere.");
   return NS_OK;
 }
 
--- a/toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.h
+++ b/toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.h
@@ -76,17 +76,25 @@ private:
 
   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;
+
+  // In v2, a update response might contain redirection and this
+  // timer is for fetching the redirected update.
+  nsCOMPtr<nsITimer> mFetchIndirectUpdatesTimer;
+
+  // When we DownloadUpdate(), the DBService might be busy on processing
+  // request issused outside of StreamUpdater. We have to fire a timer to
+  // retry on our own.
+  nsCOMPtr<nsITimer> mFetchNextRequestTimer;
 
   struct PendingRequest {
     nsCString mTables;
     nsCString mRequestPayload;
     bool mIsPostRequest;
     nsCString mUrl;
     nsCOMPtr<nsIUrlClassifierCallback> mSuccessCallback;
     nsCOMPtr<nsIUrlClassifierCallback> mUpdateErrorCallback;