Bug 1562822 - P3. Reset all the tables that fail to apply a Safe Browsing update. r=gcp
authordlee <dlee@mozilla.com>
Wed, 21 Aug 2019 12:08:12 +0000
changeset 489175 df6b5b4da8b909d61701356243dd95f20e983bf0
parent 489174 d49cd2aa45686212954493e2465fc3258c1796a2
child 489176 d359a7ccf59750a2a2e145054227b72239cc9615
push id36465
push userdvarga@mozilla.com
push dateWed, 21 Aug 2019 16:47:43 +0000
treeherdermozilla-central@4ab60925635c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersgcp
bugs1562822
milestone70.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 1562822 - P3. Reset all the tables that fail to apply a Safe Browsing update. r=gcp Before this patch, when Safe Browsing updating process discovers an error, it quits and resets the table failing to update. After this patch, updating process will continue to run when an error occurs to find all the tables failing to apply an update. Differential Revision: https://phabricator.services.mozilla.com/D42615
toolkit/components/url-classifier/Classifier.cpp
toolkit/components/url-classifier/Classifier.h
--- a/toolkit/components/url-classifier/Classifier.cpp
+++ b/toolkit/components/url-classifier/Classifier.cpp
@@ -724,52 +724,53 @@ nsresult Classifier::AsyncApplyUpdates(c
   RefPtr<Classifier> self = this;
   nsCOMPtr<nsIRunnable> bgRunnable = NS_NewRunnableFunction(
       "safebrowsing::Classifier::AsyncApplyUpdates",
       [self, aUpdates, aCallback, callerThread] {
         MOZ_ASSERT(NS_GetCurrentThread() == self->mUpdateThread,
                    "MUST be on update thread");
 
         nsresult bgRv;
-        nsCString failedTableName;
+        nsTArray<nsCString> failedTableNames;
 
         TableUpdateArray updates;
 
         // Make a copy of the array since we'll be removing entries as
         // we process them on the background thread.
         if (updates.AppendElements(aUpdates, fallible)) {
           LOG(("Step 1. ApplyUpdatesBackground on update thread."));
-          bgRv = self->ApplyUpdatesBackground(updates, failedTableName);
+          bgRv = self->ApplyUpdatesBackground(updates, failedTableNames);
         } else {
           LOG(
               ("Step 1. Not enough memory to run ApplyUpdatesBackground on "
                "update thread."));
           bgRv = NS_ERROR_OUT_OF_MEMORY;
         }
 
         nsCOMPtr<nsIRunnable> fgRunnable = NS_NewRunnableFunction(
             "safebrowsing::Classifier::AsyncApplyUpdates",
-            [self, aCallback, bgRv, failedTableName, callerThread] {
+            [self, aCallback, bgRv, failedTableNames, callerThread] {
               MOZ_ASSERT(NS_GetCurrentThread() == callerThread,
                          "MUST be on caller thread");
 
               LOG(("Step 2. ApplyUpdatesForeground on caller thread"));
-              nsresult rv = self->ApplyUpdatesForeground(bgRv, failedTableName);
+              nsresult rv =
+                  self->ApplyUpdatesForeground(bgRv, failedTableNames);
 
               LOG(("Step 3. Updates applied! Fire callback."));
               aCallback(rv);
             });
         callerThread->Dispatch(fgRunnable, NS_DISPATCH_NORMAL);
       });
 
   return mUpdateThread->Dispatch(bgRunnable, NS_DISPATCH_NORMAL);
 }
 
-nsresult Classifier::ApplyUpdatesBackground(TableUpdateArray& aUpdates,
-                                            nsACString& aFailedTableName) {
+nsresult Classifier::ApplyUpdatesBackground(
+    TableUpdateArray& aUpdates, nsTArray<nsCString>& aFailedTableNames) {
   // |mUpdateInterrupted| is guaranteed to have been unset.
   // If |mUpdateInterrupted| is set at any point, Reset() must have
   // been called then we need to interrupt the update process.
   // We only add checkpoints for non-trivial tasks.
 
   if (aUpdates.IsEmpty()) {
     return NS_OK;
   }
@@ -821,48 +822,62 @@ nsresult Classifier::ApplyUpdatesBackgro
 
     // Will update the mirrored in-memory and on-disk databases.
     if (TableUpdate::Cast<TableUpdateV2>(update)) {
       rv = UpdateHashStore(aUpdates, updateTable);
     } else {
       rv = UpdateTableV4(aUpdates, updateTable);
     }
 
-    if (NS_FAILED(rv)) {
-      aFailedTableName = updateTable;
-      RemoveUpdateIntermediaries();
-      return rv;
+    if (NS_WARN_IF(NS_FAILED(rv))) {
+      LOG(("Failed to update table: %s", updateTable.get()));
+      // We don't quit the updating process immediately when we discover
+      // a failure. Instead, we continue to apply updates to the
+      // remaining tables to find other tables which may also fail to
+      // apply an update. This help us reset all the corrupted tables
+      // within a single update.
+      // Note that changes that result from successful updates don't take
+      // effect after the updating process is finished. This is because
+      // when an error occurs during the updating process, we ignore all
+      // changes that have happened during the udpating process.
+      aFailedTableNames.AppendElement(updateTable);
+      continue;
     }
   }
 
+  if (!aFailedTableNames.IsEmpty()) {
+    RemoveUpdateIntermediaries();
+    return NS_ERROR_FAILURE;
+  }
+
   if (LOG_ENABLED()) {
     PRIntervalTime clockEnd = PR_IntervalNow();
     LOG(("update took %dms\n",
          PR_IntervalToMilliseconds(clockEnd - clockStart)));
   }
 
   return rv;
 }
 
 nsresult Classifier::ApplyUpdatesForeground(
-    nsresult aBackgroundRv, const nsACString& aFailedTableName) {
+    nsresult aBackgroundRv, const nsTArray<nsCString>& aFailedTableNames) {
   if (ShouldAbort()) {
     LOG(("Update is interrupted! Just remove update intermediaries."));
     RemoveUpdateIntermediaries();
     return NS_OK;
   }
   if (NS_SUCCEEDED(aBackgroundRv)) {
     // Copy and Invalidate fullhash cache here because this call requires
     // mLookupCaches which is only available on work-thread
     CopyAndInvalidateFullHashCache();
 
     return SwapInNewTablesAndCleanup();
   }
   if (NS_ERROR_OUT_OF_MEMORY != aBackgroundRv) {
-    ResetTables(Clear_All, nsTArray<nsCString>{nsCString(aFailedTableName)});
+    ResetTables(Clear_All, aFailedTableNames);
   }
   return aBackgroundRv;
 }
 
 nsresult Classifier::ApplyFullHashes(ConstTableUpdateArray& aUpdates) {
   MOZ_ASSERT(NS_GetCurrentThread() != mUpdateThread,
              "ApplyFullHashes() MUST NOT be called on update thread");
   MOZ_ASSERT(
--- a/toolkit/components/url-classifier/Classifier.h
+++ b/toolkit/components/url-classifier/Classifier.h
@@ -182,30 +182,30 @@ class Classifier {
 
   /**
    * The "background" part of ApplyUpdates. Once the background update
    * is called, the foreground update has to be called along with the
    * background result no matter whether the background update is
    * successful or not.
    */
   nsresult ApplyUpdatesBackground(TableUpdateArray& aUpdates,
-                                  nsACString& aFailedTableName);
+                                  nsTArray<nsCString>& aFailedTableNames);
 
   /**
    * The "foreground" part of ApplyUpdates. The in-use data (in-memory and
    * on-disk) will be touched so this MUST be mutually exclusive to other
    * member functions.
    *
    * If |aBackgroundRv| is successful, the return value is the result of
    * bringing stuff to the foreground. Otherwise, the foreground table may
    * be reset according to the background update failed reason and
    * |aBackgroundRv| will be returned to forward the background update result.
    */
   nsresult ApplyUpdatesForeground(nsresult aBackgroundRv,
-                                  const nsACString& aFailedTableName);
+                                  const nsTArray<nsCString>& aFailedTableNames);
 
   // Used by worker thread and update thread to abort current operation.
   bool ShouldAbort() const;
 
   // Add built-in entries for testing.
   nsresult AddMozEntries(nsTArray<nsCString>& aTables);
 
   // Remove test files if exist