Bug 1339760 - Split update process to background/foreground and run background on update thread **synchronously**. draft
authorHenry Chang <hchang@mozilla.com>
Wed, 22 Feb 2017 17:25:26 +0800
changeset 494603 40c38b36dba2ce9a2070c9f120aec65cc720de4a
parent 494545 f50437ed4599e0065dce3e0e08b948fb13ea2664
child 548150 06c771211e02cd77a65551ec487ae47b844391fc
push id48083
push userhchang@mozilla.com
push dateTue, 07 Mar 2017 16:01:58 +0000
bugs1339760
milestone54.0a1
Bug 1339760 - Split update process to background/foreground and run background on update thread **synchronously**. MozReview-Commit-ID: J0phPC1nWsf
toolkit/components/url-classifier/Classifier.cpp
toolkit/components/url-classifier/Classifier.h
toolkit/components/url-classifier/nsUrlClassifierDBService.cpp
toolkit/components/url-classifier/nsUrlClassifierDBService.h
--- a/toolkit/components/url-classifier/Classifier.cpp
+++ b/toolkit/components/url-classifier/Classifier.cpp
@@ -350,30 +350,16 @@ Classifier::DeleteTables(nsIFile* aDirec
                                    leafName.get()).get());
       }
     }
   }
   NS_ENSURE_SUCCESS_VOID(rv);
 }
 
 void
-Classifier::AbortUpdateAndReset(const nsCString& aTable)
-{
-  // We don't need to reset while shutting down. It will only slow us down.
-  if (nsUrlClassifierDBService::ShutdownHasStarted()) {
-    return;
-  }
-
-  LOG(("Abort updating table %s.", aTable.get()));
-
-  // ResetTables will clear both in-memory & on-disk data.
-  ResetTables(Clear_All, nsTArray<nsCString> { aTable });
-}
-
-void
 Classifier::TableRequest(nsACString& aResult)
 {
   MOZ_ASSERT(!NS_IsMainThread(),
              "TableRequest must be called on the classifier worker thread.");
 
   // This function and all disk I/O are guaranteed to occur
   // on the same thread so we don't need to add a lock around.
   if (!mIsTableRequestResultOutdated) {
@@ -689,16 +675,26 @@ Classifier::SwapInNewTablesAndCleanup()
   LOG(("Done swap in updated tables."));
 
   return rv;
 }
 
 nsresult
 Classifier::ApplyUpdates(nsTArray<TableUpdate*>* aUpdates)
 {
+  nsCString failedTableName;
+
+  nsresult bgRv = ApplyUpdatesBackground(aUpdates, failedTableName);
+  return ApplyUpdatesForeground(bgRv, failedTableName);
+}
+
+nsresult
+Classifier::ApplyUpdatesBackground(nsTArray<TableUpdate*>* aUpdates,
+                                   nsACString& aFailedTableName)
+{
   // Will run on the update thread after Bug 1339760.
   // No lock is required except for CopyInUseDirForUpdate() since
   // the table lookup may lead to database removal.
 
   if (!aUpdates || aUpdates->Length() == 0) {
     return NS_OK;
   }
 
@@ -750,46 +746,53 @@ Classifier::ApplyUpdates(nsTArray<TableU
         // Will update the mirrored in-memory and on-disk databases.
         if (TableUpdate::Cast<TableUpdateV2>((*aUpdates)[i])) {
           rv = UpdateHashStore(aUpdates, updateTable);
         } else {
           rv = UpdateTableV4(aUpdates, updateTable);
         }
 
         if (NS_FAILED(rv)) {
+          aFailedTableName = updateTable;
           if (rv != NS_ERROR_OUT_OF_MEMORY) {
 #ifdef MOZ_SAFEBROWSING_DUMP_FAILED_UPDATES
             DumpFailedUpdate();
 #endif
-            // TODO: Bug 1339050 - Should be run on worker thread.
-            AbortUpdateAndReset(updateTable);
           }
           RemoveUpdateIntermediaries();
           return rv;
         }
       }
     }
 
   } // End of scopedUpdatesClearer scope.
 
-  rv = SwapInNewTablesAndCleanup();
-  if (NS_FAILED(rv)) {
-    LOG(("Failed to swap in new tables."));
-  }
-
   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)
+{
+  if (NS_SUCCEEDED(aBackgroundRv)) {
+    return SwapInNewTablesAndCleanup();
+  }
+  if (NS_ERROR_OUT_OF_MEMORY != aBackgroundRv) {
+    ResetTables(Clear_All, nsTArray<nsCString> { nsCString(aFailedTableName) });
+  }
+  return aBackgroundRv;
+}
+
+nsresult
 Classifier::ApplyFullHashes(nsTArray<TableUpdate*>* aUpdates)
 {
   LOG(("Applying %" PRIuSIZE " table gethashes.", aUpdates->Length()));
 
   ScopedUpdatesClearer scopedUpdatesClearer(aUpdates);
   for (uint32_t i = 0; i < aUpdates->Length(); i++) {
     TableUpdate *update = aUpdates->ElementAt(i);
 
--- a/toolkit/components/url-classifier/Classifier.h
+++ b/toolkit/components/url-classifier/Classifier.h
@@ -66,16 +66,38 @@ public:
 
   /**
    * Apply the table updates in the array.  Takes ownership of
    * the updates in the array and clears it.  Wacky!
    */
   nsresult ApplyUpdates(nsTArray<TableUpdate*>* aUpdates);
 
   /**
+   * 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(nsTArray<TableUpdate*>* aUpdates,
+                                  nsACString& aFailedTableName);
+
+  /**
+   * 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);
+
+  /**
    * Apply full hashes retrived from gethash to cache.
    */
   nsresult ApplyFullHashes(nsTArray<TableUpdate*>* aUpdates);
 
   void SetLastUpdateTime(const nsACString& aTableName, uint64_t updateTime);
   int64_t GetLastUpdateTime(const nsACString& aTableName);
   nsresult CacheCompletions(const CacheResultArray& aResults);
   uint32_t GetHashKey(void) { return mHashKey; }
@@ -114,17 +136,16 @@ public:
   nsresult SwapInNewTablesAndCleanup();
 
   LookupCache *GetLookupCache(const nsACString& aTable,
                               bool aForUpdate = false);
 
 private:
   void DropStores();
   void DeleteTables(nsIFile* aDirectory, const nsTArray<nsCString>& aTables);
-  void AbortUpdateAndReset(const nsCString& aTable);
 
   nsresult CreateStoreDirectory();
   nsresult SetupPathNames();
   nsresult RecoverBackups();
   nsresult CleanToDelete();
   nsresult CopyInUseDirForUpdate();
   nsresult CopyInUseLookupCacheForUpdate();
   nsresult RegenActiveTables();
--- a/toolkit/components/url-classifier/nsUrlClassifierDBService.cpp
+++ b/toolkit/components/url-classifier/nsUrlClassifierDBService.cpp
@@ -130,16 +130,19 @@ LazyLogModule gUrlClassifierDbServiceLog
 
 class nsUrlClassifierDBServiceWorker;
 
 // Singleton instance.
 static nsUrlClassifierDBService* sUrlClassifierDBService;
 
 nsIThread* nsUrlClassifierDBService::gDbBackgroundThread = nullptr;
 
+// For update only.
+static nsIThread* gDbUpdateThread = nullptr;
+
 // Once we've committed to shutting down, don't do work in the background
 // thread.
 static bool gShuttingDownThread = false;
 
 static mozilla::Atomic<int32_t> gFreshnessGuarantee(CONFIRM_AGE_DEFAULT_SEC);
 
 NS_IMPL_ISUPPORTS(nsUrlClassifierDBServiceWorker,
                   nsIUrlClassifierDBService)
@@ -607,25 +610,83 @@ nsUrlClassifierDBServiceWorker::FinishSt
 
 NS_IMETHODIMP
 nsUrlClassifierDBServiceWorker::FinishUpdate()
 {
   if (gShuttingDownThread) {
     return NS_ERROR_NOT_INITIALIZED;
   }
 
+  MOZ_ASSERT(!mProtocolParser, "Should have been nulled out in FinishStream() "
+                               "or never created.");
+
   NS_ENSURE_STATE(mUpdateObserver);
 
-  if (NS_SUCCEEDED(mUpdateStatus)) {
-    mUpdateStatus = ApplyUpdate();
-  } else {
+  if (NS_FAILED(mUpdateStatus)) {
     LOG(("nsUrlClassifierDBServiceWorker::FinishUpdate() Not running "
          "ApplyUpdate() since the update has already failed."));
+    return NotifyUpdateObserver(mUpdateStatus);
   }
 
+  if (mTableUpdates.IsEmpty()) {
+    LOG(("Nothing to update. Just notify update observer."));
+    return NotifyUpdateObserver(NS_OK);
+  }
+
+  RefPtr<nsUrlClassifierDBServiceWorker> self = this;
+
+  // TODO: Asynchronously dispatch |ApplyUpdatesBackground| to update thread.
+  //       See Bug 1339050. For now we *synchronously* run
+  //       ApplyUpdatesForeground() after ApplyUpdatesBackground().
+  nsresult backgroundRv;
+  nsCString failedTableName;
+  nsCOMPtr<nsIRunnable> r =
+    NS_NewRunnableFunction([self, &backgroundRv, &failedTableName] () -> void {
+      backgroundRv = self->ApplyUpdatesBackground(failedTableName);
+    });
+
+  LOG(("Bulding new tables..."));
+  mozilla::SyncRunnable::DispatchToThread(gDbUpdateThread, r);
+  LOG(("Done bulding new tables. Try to commit them."));
+
+  return ApplyUpdatesForeground(backgroundRv, failedTableName);
+}
+
+nsresult
+nsUrlClassifierDBServiceWorker::ApplyUpdatesForeground(nsresult aBackgroundRv,
+                                                      const nsACString& aFailedTableName)
+{
+  if (gShuttingDownThread) {
+    return NS_ERROR_NOT_INITIALIZED;
+  }
+
+  MOZ_ASSERT(NS_GetCurrentThread() == nsUrlClassifierDBService::BackgroundThread(),
+             "nsUrlClassifierDBServiceWorker::ApplyUpdatesForeground MUST "
+             "run on worker thread!!");
+
+  nsresult rv =
+    mClassifier->ApplyUpdatesForeground(aBackgroundRv, aFailedTableName);
+
+  return NotifyUpdateObserver(rv);
+}
+
+nsresult
+nsUrlClassifierDBServiceWorker::NotifyUpdateObserver(nsresult aUpdateStatus)
+{
+  // We've either
+  //  1) failed starting a download stream
+  //  2) succeeded in starting a download stream but failed to obtain
+  //     table updates
+  //  3) succeeded in obtaining table updates but failed to build new
+  //     tables.
+  //  4) succeeded in building new tables but failed to take them.
+  //  5) succeeded in taking new tables.
+
+  mUpdateStatus = aUpdateStatus;
+
   nsCOMPtr<nsIUrlClassifierUtils> urlUtil =
     do_GetService(NS_URLCLASSIFIERUTILS_CONTRACTID);
 
   nsCString provider;
   // Assume that all the tables in update should have the same provider.
   urlUtil->GetTelemetryProvider(mUpdateTables.SafeElementAt(0, EmptyCString()), provider);
 
   nsresult updateStatus = mUpdateStatus;
@@ -666,20 +727,34 @@ nsUrlClassifierDBServiceWorker::FinishUp
     mClassifier->ResetTables(Classifier::Clear_Cache, mUpdateTables);
   }
   mUpdateObserver = nullptr;
 
   return NS_OK;
 }
 
 nsresult
-nsUrlClassifierDBServiceWorker::ApplyUpdate()
+nsUrlClassifierDBServiceWorker::ApplyUpdatesBackground(nsACString& aFailedTableName)
 {
-  LOG(("nsUrlClassifierDBServiceWorker::ApplyUpdate()"));
-  nsresult rv = mClassifier->ApplyUpdates(&mTableUpdates);
+  // ********* Please be very careful while adding tasks. *********
+  // This function will run on the update thread (whereas most of the other
+  // functions will run on the worker thread) so any task that might mutate
+  // the in-use data is forbidden.
+
+  if (gShuttingDownThread) {
+    return NS_ERROR_NOT_INITIALIZED;
+  }
+
+  MOZ_ASSERT(NS_GetCurrentThread() == gDbUpdateThread,
+             "nsUrlClassifierDBServiceWorker::BuildNewTables MUST "
+             "run on update thread!!");
+
+  LOG(("nsUrlClassifierDBServiceWorker::BuildNewTables()"));
+  nsresult rv = mClassifier->ApplyUpdatesBackground(&mTableUpdates,
+                                                    aFailedTableName);
 
 #ifdef MOZ_SAFEBROWSING_DUMP_FAILED_UPDATES
   if (NS_FAILED(rv) && NS_ERROR_OUT_OF_MEMORY != rv) {
     mClassifier->DumpRawTableUpdates(mRawTableUpdates);
   }
   // Invalidate the raw table updates.
   mRawTableUpdates = EmptyCString();
 #endif
@@ -1509,16 +1584,23 @@ nsUrlClassifierDBService::Init()
     }
   }
 
   // Start the background thread.
   rv = NS_NewNamedThread("URL Classifier", &gDbBackgroundThread);
   if (NS_FAILED(rv))
     return rv;
 
+  // Start the update thread.
+  rv = NS_NewNamedThread(NS_LITERAL_CSTRING("SafeBrowsing DB Update"),
+                         &gDbUpdateThread);
+  if (NS_FAILED(rv)) {
+    return rv;
+  }
+
   mWorker = new nsUrlClassifierDBServiceWorker();
   if (!mWorker)
     return NS_ERROR_OUT_OF_MEMORY;
 
   rv = mWorker->Init(gethashNoise, cacheDir);
   if (NS_FAILED(rv)) {
     mWorker = nullptr;
     return rv;
@@ -2088,40 +2170,50 @@ nsUrlClassifierDBService::Observe(nsISup
     Shutdown();
   } else if (!strcmp(aTopic, "profile-before-change")) {
     // Unit test does not receive "quit-application",
     // need call shutdown in this case
     Shutdown();
     LOG(("joining background thread"));
     mWorkerProxy = nullptr;
 
-    if (!gDbBackgroundThread) {
-      return NS_OK;
+    if (gDbBackgroundThread) {
+      nsIThread *backgroundThread = gDbBackgroundThread;
+      // Nulling out gDbBackgroundThread MUST be done before Shutdown()
+      // since Shutdown() will lead to processing pending events.
+      gDbBackgroundThread = nullptr;
+      backgroundThread->Shutdown();
+      NS_RELEASE(backgroundThread);
     }
 
-    nsIThread *backgroundThread = gDbBackgroundThread;
-    gDbBackgroundThread = nullptr;
-    backgroundThread->Shutdown();
-    NS_RELEASE(backgroundThread);
+    if (gDbUpdateThread) {
+      nsIThread *updateThread = gDbUpdateThread;
+      gDbUpdateThread = nullptr;
+      updateThread->Shutdown();
+      NS_RELEASE(updateThread);
+    }
+
+    return NS_OK;
   } else {
     return NS_ERROR_UNEXPECTED;
   }
 
   return NS_OK;
 }
 
 // Join the background thread if it exists.
 nsresult
 nsUrlClassifierDBService::Shutdown()
 {
   LOG(("shutting down db service\n"));
   MOZ_ASSERT(XRE_IsParentProcess());
 
-  if (!gDbBackgroundThread || gShuttingDownThread)
+  if ((!gDbBackgroundThread && !gDbUpdateThread) || gShuttingDownThread) {
     return NS_OK;
+  }
 
   gShuttingDownThread = true;
 
   Telemetry::AutoTimer<Telemetry::URLCLASSIFIER_SHUTDOWN_TIME> timer;
 
   mCompleters.Clear();
 
   nsCOMPtr<nsIPrefBranch> prefs = do_GetService(NS_PREFSERVICE_CONTRACTID);
--- a/toolkit/components/url-classifier/nsUrlClassifierDBService.h
+++ b/toolkit/components/url-classifier/nsUrlClassifierDBService.h
@@ -184,18 +184,24 @@ public:
 
 private:
   // No subclassing
   ~nsUrlClassifierDBServiceWorker();
 
   // Disallow copy constructor
   nsUrlClassifierDBServiceWorker(nsUrlClassifierDBServiceWorker&);
 
-  // Applies the current transaction and resets the update/working times.
-  nsresult ApplyUpdate();
+  // Perform update in the background. Can be run on/off the worker thread.
+  nsresult ApplyUpdatesBackground(nsACString& aFailedTableName);
+
+  // Perform update for the foreground part. MUST be run on the worker thread.
+  nsresult ApplyUpdatesForeground(nsresult aBackgroundRv,
+                                 const nsACString& aFailedTableName);
+
+  nsresult NotifyUpdateObserver(nsresult aUpdateStatus);
 
   // Reset the in-progress update stream
   void ResetStream();
 
   // Reset the in-progress update
   void ResetUpdate();
 
   // Perform a classifier lookup for a given url.