Bug 1296820 - Skip applying TableUpdateV4 to avoid premature update codes being run. r=francois draft
authorHenry Chang <hchang@mozilla.com>
Fri, 26 Aug 2016 14:35:53 +0800
changeset 405949 f3f9ad52c0d6a690e1ce9c8b47cc7bfe848414e2
parent 403581 f97a056ae6235de7855fd8aaa04fb1c8d183bd06
child 529549 7ea4c43fb169ae83fda003017d6b314c78f69ed5
push id27610
push userhchang@mozilla.com
push dateFri, 26 Aug 2016 06:54:25 +0000
reviewersfrancois
bugs1296820
milestone51.0a1
Bug 1296820 - Skip applying TableUpdateV4 to avoid premature update codes being run. r=francois MozReview-Commit-ID: IqjpAVgISLJ
toolkit/components/url-classifier/Classifier.cpp
toolkit/components/url-classifier/nsUrlClassifierDBService.cpp
--- a/toolkit/components/url-classifier/Classifier.cpp
+++ b/toolkit/components/url-classifier/Classifier.cpp
@@ -23,16 +23,43 @@ extern mozilla::LazyLogModule gUrlClassi
 
 #define STORE_DIRECTORY      NS_LITERAL_CSTRING("safebrowsing")
 #define TO_DELETE_DIR_SUFFIX NS_LITERAL_CSTRING("-to_delete")
 #define BACKUP_DIR_SUFFIX    NS_LITERAL_CSTRING("-backup")
 
 namespace mozilla {
 namespace safebrowsing {
 
+namespace {
+
+// A scoped-clearer for nsTArray<TableUpdate*>.
+// The owning elements will be deleted and the array itself
+// will be cleared on exiting the scope.
+class ScopedUpdatesClearer {
+public:
+  explicit ScopedUpdatesClearer(nsTArray<TableUpdate*> *aUpdates)
+    : mUpdatesArrayRef(aUpdates)
+  {
+    for (auto update : *aUpdates) {
+      mUpdatesPointerHolder.AppendElement(update);
+    }
+  }
+
+  ~ScopedUpdatesClearer()
+  {
+    mUpdatesArrayRef->Clear();
+  }
+
+private:
+  nsTArray<TableUpdate*>* mUpdatesArrayRef;
+  nsTArray<nsAutoPtr<TableUpdate>> mUpdatesPointerHolder;
+};
+
+} // End of unnamed namespace.
+
 void
 Classifier::SplitTables(const nsACString& str, nsTArray<nsCString>& tables)
 {
   tables.Clear();
 
   nsACString::const_iterator begin, iter, end;
   str.BeginReading(begin);
   str.EndReading(end);
@@ -291,38 +318,55 @@ Classifier::ApplyUpdates(nsTArray<TableU
 {
   Telemetry::AutoTimer<Telemetry::URLCLASSIFIER_CL_UPDATE_TIME> timer;
 
   PRIntervalTime clockStart = 0;
   if (LOG_ENABLED()) {
     clockStart = PR_IntervalNow();
   }
 
-  LOG(("Backup before update."));
+  nsresult rv;
 
-  nsresult rv = BackupTables();
-  NS_ENSURE_SUCCESS(rv, rv);
-
-  LOG(("Applying %d table updates.", aUpdates->Length()));
+  {
+    ScopedUpdatesClearer scopedUpdatesClearer(aUpdates);
 
-  for (uint32_t i = 0; i < aUpdates->Length(); i++) {
-    // Previous UpdateHashStore() may have consumed this update..
-    if ((*aUpdates)[i]) {
-      // Run all updates for one table
-      nsCString updateTable(aUpdates->ElementAt(i)->TableName());
-      rv = UpdateHashStore(aUpdates, updateTable);
-      if (NS_FAILED(rv)) {
-        if (rv != NS_ERROR_OUT_OF_MEMORY) {
-          Reset();
-        }
-        return rv;
+    // In order to prevent any premature update code from being
+    // run against V4 updates, we bail out as early as possible
+    // if aUpdates is using V4.
+    for (auto update : *aUpdates) {
+      if (update && TableUpdate::Cast<TableUpdateV4>(update)) {
+        LOG(("V4 update is not supported yet."));
+        // TODO: Bug 1283009 - Supports applying table udpate V4.
+        return NS_ERROR_NOT_IMPLEMENTED;
       }
     }
-  }
-  aUpdates->Clear();
+
+    LOG(("Backup before update."));
+
+    rv = BackupTables();
+    NS_ENSURE_SUCCESS(rv, rv);
+
+    LOG(("Applying %d table updates.", aUpdates->Length()));
+
+    for (uint32_t i = 0; i < aUpdates->Length(); i++) {
+      // Previous UpdateHashStore() may have consumed this update..
+      if ((*aUpdates)[i]) {
+        // Run all updates for one table
+        nsCString updateTable(aUpdates->ElementAt(i)->TableName());
+        rv = UpdateHashStore(aUpdates, updateTable);
+        if (NS_FAILED(rv)) {
+          if (rv != NS_ERROR_OUT_OF_MEMORY) {
+            Reset();
+          }
+          return rv;
+        }
+      }
+    }
+
+  } // End of scopedUpdatesClearer scope.
 
   rv = RegenActiveTables();
   NS_ENSURE_SUCCESS(rv, rv);
 
   LOG(("Cleaning up backups."));
 
   // Move the backup directory away (signaling the transaction finished
   // successfully). This is atomic.
@@ -344,26 +388,25 @@ Classifier::ApplyUpdates(nsTArray<TableU
   return NS_OK;
 }
 
 nsresult
 Classifier::ApplyFullHashes(nsTArray<TableUpdate*>* aUpdates)
 {
   LOG(("Applying %d table gethashes.", aUpdates->Length()));
 
+  ScopedUpdatesClearer scopedUpdatesClearer(aUpdates);
   for (uint32_t i = 0; i < aUpdates->Length(); i++) {
     TableUpdate *update = aUpdates->ElementAt(i);
 
     nsresult rv = UpdateCache(update);
     NS_ENSURE_SUCCESS(rv, rv);
 
     aUpdates->ElementAt(i) = nullptr;
-    delete update;
   }
-  aUpdates->Clear();
 
   return NS_OK;
 }
 
 nsresult
 Classifier::MarkSpoiled(nsTArray<nsCString>& aTables)
 {
   for (uint32_t i = 0; i < aTables.Length(); i++) {
@@ -586,17 +629,16 @@ Classifier::CheckValidUpdate(nsTArray<Ta
   uint32_t validupdates = 0;
 
   for (uint32_t i = 0; i < aUpdates->Length(); i++) {
     TableUpdate *update = aUpdates->ElementAt(i);
     if (!update || !update->TableName().Equals(aTable))
       continue;
     if (update->Empty()) {
       aUpdates->ElementAt(i) = nullptr;
-      delete update;
       continue;
     }
     validupdates++;
   }
 
   if (!validupdates) {
     // This can happen if the update was only valid for one table.
     return false;
@@ -662,17 +704,16 @@ Classifier::UpdateHashStore(nsTArray<Tab
       LOG(("  %d sub chunks", updateV2->SubChunks().Length()));
       LOG(("  %d sub prefixes", updateV2->SubPrefixes().Length()));
       LOG(("  %d sub completions", updateV2->SubCompletes().Length()));
       LOG(("  %d add expirations", updateV2->AddExpirations().Length()));
       LOG(("  %d sub expirations", updateV2->SubExpirations().Length()));
     }
 
     aUpdates->ElementAt(i) = nullptr;
-    delete update;
   }
 
   LOG(("Applied %d update(s) to %s.", applied, store.TableName().get()));
 
   rv = store.Rebuild();
   NS_ENSURE_SUCCESS(rv, rv);
 
   LOG(("Table %s now has:", store.TableName().get()));
--- a/toolkit/components/url-classifier/nsUrlClassifierDBService.cpp
+++ b/toolkit/components/url-classifier/nsUrlClassifierDBService.cpp
@@ -581,16 +581,21 @@ nsUrlClassifierDBServiceWorker::FinishUp
          "ApplyUpdate() since the update has already failed."));
   }
 
   mMissCache.Clear();
 
   if (NS_SUCCEEDED(mUpdateStatus)) {
     LOG(("Notifying success: %d", mUpdateWait));
     mUpdateObserver->UpdateSuccess(mUpdateWait);
+  } else if (NS_ERROR_NOT_IMPLEMENTED == mUpdateStatus) {
+    LOG(("Treating NS_ERROR_NOT_IMPLEMENTED a successful update "
+         "but still mark it spoiled."));
+    mUpdateObserver->UpdateSuccess(0);
+    mClassifier->MarkSpoiled(mUpdateTables);
   } else {
     if (LOG_ENABLED()) {
       nsAutoCString errorName;
       mozilla::GetErrorName(mUpdateStatus, errorName);
       LOG(("Notifying error: %s (%d)", errorName.get(), mUpdateStatus));
     }
 
     mUpdateObserver->UpdateError(mUpdateStatus);