Bug 1504774 - Fix url-classifier worker thread is not aborted while shutting down. r=francois, a=RyanVM
authordlee <dlee@mozilla.com>
Wed, 19 Dec 2018 10:03:19 +0000
changeset 509268 b168e29f3c821f8d170d99c42bec442eb32351de
parent 509267 c511b04377a198c6da03b32ea27e7d042e75fda1
child 509269 fea77d171e0f3de696f7525018a1da062aab22e5
push id1905
push userffxbld-merge
push dateMon, 21 Jan 2019 12:33:13 +0000
treeherdermozilla-release@c2fca1944d8c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersfrancois, RyanVM
bugs1504774, 1453038
milestone65.0
Bug 1504774 - Fix url-classifier worker thread is not aborted while shutting down. r=francois, a=RyanVM In Bug 1453038, |mUpdateInterrupted| is set in Classifer::Close() which is called by PreShutdown to abort an ongoing update. That doesn't handle all the cases. The SafeBrowsing update involves two threads, worker thread, and update thread. Update thread takes care of most of the update work, when it finishes its task, it posts a task back to the worker thread to apply the updated database and also do some cleanup stuff. Then the update is complete. The fix in Bug 1453038 doesn't abort an update if the woker thread is doing the job. This is because the |mUpdateInterrupted| flag is set in the worker thread. The PreShutdown event which eventually sets the flag has to wait until the worker thread's current task is done. In this patch: 1. Check nsUrlClassifierDBService::ShutdownHasStarted() to abort shutdown. This is set by main thread so both worker thread and update thread can be interrupted now. 2. mIsClosed is now replaced by the mUpdateInterrupted. The semantics of mUpdateInterrupted is now changed to abort update for any internal APIs which should cause an update to abort. 3. Remove |mUpdateInterrupted| and |ShutdownHasStarted()| checks and unify with |ShouldAbort()| Differential Revision: https://phabricator.services.mozilla.com/D12229
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
@@ -169,17 +169,17 @@ nsresult Classifier::SetupPathNames() {
 
   rv = mToDeleteDirectory->AppendNative(STORE_DIRECTORY + TO_DELETE_DIR_SUFFIX);
   NS_ENSURE_SUCCESS(rv, rv);
 
   return NS_OK;
 }
 
 nsresult Classifier::CreateStoreDirectory() {
-  if (mIsClosed) {
+  if (ShouldAbort()) {
     return NS_OK;  // nothing to do, the classifier is done
   }
 
   // Ensure the safebrowsing directory exists.
   bool storeExists;
   nsresult rv = mRootStoreDirectory->Exists(&storeExists);
   NS_ENSURE_SUCCESS(rv, rv);
 
@@ -231,20 +231,18 @@ nsresult Classifier::Open(nsIFile& aCach
   // Build the list of know urlclassifier lists
   // XXX: Disk IO potentially on the main thread during startup
   RegenActiveTables();
 
   return NS_OK;
 }
 
 void Classifier::Close() {
-  // Close will be called by PreShutdown, set |mUpdateInterrupted| here
-  // to abort an ongoing update if possible. It is important to note that
+  // Close will be called by PreShutdown, so it is important to note that
   // things put here should not affect an ongoing update thread.
-  mUpdateInterrupted = true;
   mIsClosed = true;
   DropStores();
 }
 
 void Classifier::Reset() {
   MOZ_ASSERT(NS_GetCurrentThread() != mUpdateThread,
              "Reset() MUST NOT be called on update thread");
 
@@ -749,17 +747,17 @@ nsresult Classifier::ApplyUpdatesBackgro
 
   PRIntervalTime clockStart = 0;
   if (LOG_ENABLED()) {
     clockStart = PR_IntervalNow();
   }
 
   nsresult rv;
 
-  // Check point 1: Copying files takes time so we check |mUpdateInterrupted|
+  // Check point 1: Copying files takes time so we check ShouldAbort()
   //                inside CopyInUseDirForUpdate().
   rv = CopyInUseDirForUpdate();  // i.e. mUpdatingDirectory will be setup.
   if (NS_FAILED(rv)) {
     LOG(("Failed to copy in-use directory for update."));
     return (rv == NS_ERROR_ABORT) ? NS_OK : rv;
   }
 
   LOG(("Applying %zu table updates.", aUpdates.Length()));
@@ -770,17 +768,17 @@ nsresult Classifier::ApplyUpdatesBackgro
       // Previous UpdateHashStore() may have consumed this update..
       continue;
     }
 
     // Run all updates for one table
     nsAutoCString updateTable(update->TableName());
 
     // Check point 2: Processing downloaded data takes time.
-    if (mUpdateInterrupted) {
+    if (ShouldAbort()) {
       LOG(("Update is interrupted. Stop building new tables."));
       return NS_OK;
     }
 
     // Will update the mirrored in-memory and on-disk databases.
     if (TableUpdate::Cast<TableUpdateV2>(update)) {
       rv = UpdateHashStore(aUpdates, updateTable);
     } else {
@@ -800,17 +798,17 @@ nsresult Classifier::ApplyUpdatesBackgro
          PR_IntervalToMilliseconds(clockEnd - clockStart)));
   }
 
   return rv;
 }
 
 nsresult Classifier::ApplyUpdatesForeground(
     nsresult aBackgroundRv, const nsACString& aFailedTableName) {
-  if (mUpdateInterrupted) {
+  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();
@@ -853,17 +851,17 @@ void Classifier::GetCacheInfo(const nsAC
 }
 
 void Classifier::DropStores() {
   // See the comment in Classifier::Close() before adding anything here.
   mLookupCaches.Clear();
 }
 
 nsresult Classifier::RegenActiveTables() {
-  if (mIsClosed) {
+  if (ShouldAbort()) {
     return NS_OK;  // nothing to do, the classifier is done
   }
 
   mActiveTablesCache.Clear();
 
   nsTArray<nsCString> foundTables;
   ScanStoreDir(mRootStoreDirectory, foundTables);
 
@@ -1033,30 +1031,30 @@ nsresult Classifier::DumpFailedUpdate() 
 
   return rv;
 }
 
 #endif  // MOZ_SAFEBROWSING_DUMP_FAILED_UPDATES
 
 /**
  * This function copies the files one by one to the destination folder.
- * Before copying a file, it checks |mUpdateInterrupted| and returns
+ * Before copying a file, it checks ::ShouldAbort and returns
  * NS_ERROR_ABORT if the flag is set.
  */
 nsresult Classifier::CopyDirectoryInterruptible(nsCOMPtr<nsIFile>& aDestDir,
                                                 nsCOMPtr<nsIFile>& aSourceDir) {
   nsCOMPtr<nsIDirectoryEnumerator> entries;
   nsresult rv = aSourceDir->GetDirectoryEntries(getter_AddRefs(entries));
   NS_ENSURE_SUCCESS(rv, rv);
   MOZ_ASSERT(entries);
 
   nsCOMPtr<nsIFile> source;
   while (NS_SUCCEEDED(rv = entries->GetNextFile(getter_AddRefs(source))) &&
          source) {
-    if (mUpdateInterrupted) {
+    if (ShouldAbort()) {
       LOG(("Update is interrupted. Aborting the directory copy"));
       return NS_ERROR_ABORT;
     }
 
     bool isDirectory;
     rv = source->IsDirectory(&isDirectory);
     NS_ENSURE_SUCCESS(rv, rv);
 
@@ -1090,16 +1088,19 @@ nsresult Classifier::CopyDirectoryInterr
     NS_ENSURE_SUCCESS(rv, rv);
   }
 
   return NS_OK;
 }
 
 nsresult Classifier::CopyInUseDirForUpdate() {
   LOG(("Copy in-use directory content for update."));
+  if (ShouldAbort()) {
+    return NS_ERROR_UC_UPDATE_SHUTDOWNING;
+  }
 
   // We copy everything from in-use directory to a temporary directory
   // for updating.
 
   // Remove the destination directory first (just in case) the do the copy.
   mUpdatingDirectory->Remove(true);
   if (!mRootStoreDirectoryForUpdate) {
     LOG(("mRootStoreDirectoryForUpdate is null."));
@@ -1181,17 +1182,17 @@ nsCString Classifier::GetProvider(const 
   return NS_SUCCEEDED(rv) ? provider : EmptyCString();
 }
 
 /*
  * This will consume+delete updates from the passed nsTArray.
  */
 nsresult Classifier::UpdateHashStore(TableUpdateArray& aUpdates,
                                      const nsACString& aTable) {
-  if (nsUrlClassifierDBService::ShutdownHasStarted()) {
+  if (ShouldAbort()) {
     return NS_ERROR_UC_UPDATE_SHUTDOWNING;
   }
 
   LOG(("Classifier::UpdateHashStore(%s)", PromiseFlatCString(aTable).get()));
 
   HashStore store(aTable, GetProvider(aTable), mUpdatingDirectory);
 
   if (!CheckValidUpdate(aUpdates, store.TableName())) {
@@ -1283,17 +1284,17 @@ nsresult Classifier::UpdateHashStore(Tab
 
   return NS_OK;
 }
 
 nsresult Classifier::UpdateTableV4(TableUpdateArray& aUpdates,
                                    const nsACString& aTable) {
   MOZ_ASSERT(!NS_IsMainThread(),
              "UpdateTableV4 must be called on the classifier worker thread.");
-  if (nsUrlClassifierDBService::ShutdownHasStarted()) {
+  if (ShouldAbort()) {
     return NS_ERROR_UC_UPDATE_SHUTDOWNING;
   }
 
   LOG(("Classifier::UpdateTableV4(%s)", PromiseFlatCString(aTable).get()));
 
   if (!CheckValidUpdate(aUpdates, aTable)) {
     return NS_OK;
   }
@@ -1432,17 +1433,17 @@ RefPtr<LookupCache> Classifier::GetLooku
 
   for (auto c : lookupCaches) {
     if (c->TableName().Equals(aTable)) {
       return c;
     }
   }
 
   // We don't want to create lookupcache when shutdown is already happening.
-  if (nsUrlClassifierDBService::ShutdownHasStarted()) {
+  if (ShouldAbort()) {
     return nullptr;
   }
 
   // TODO : Bug 1302600, It would be better if we have a more general non-main
   //        thread method to convert table name to protocol version. Currently
   //        we can only know this by checking if the table name ends with
   //        '-proto'.
   RefPtr<LookupCache> cache;
@@ -1608,10 +1609,15 @@ nsresult Classifier::LoadMetadata(nsIFil
 
     aResult.AppendPrintf("%s;%s:%s\n", tableName.get(), stateBase64.get(),
                          checksumBase64.get());
   }
 
   return rv;
 }
 
+bool Classifier::ShouldAbort() const {
+  return mIsClosed || nsUrlClassifierDBService::ShutdownHasStarted() ||
+         (mUpdateInterrupted && (NS_GetCurrentThread() == mUpdateThread));
+}
+
 }  // namespace safebrowsing
 }  // namespace mozilla
--- a/toolkit/components/url-classifier/Classifier.h
+++ b/toolkit/components/url-classifier/Classifier.h
@@ -189,16 +189,19 @@ class Classifier {
    * 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);
 
+  // Used by worker thread and update thread to abort current operation.
+  bool ShouldAbort() const;
+
   // Root dir of the Local profile.
   nsCOMPtr<nsIFile> mCacheDirectory;
   // Main directory where to store the databases.
   nsCOMPtr<nsIFile> mRootStoreDirectory;
   // Used for atomically updating the other dirs.
   nsCOMPtr<nsIFile> mBackupDirectory;
   nsCOMPtr<nsIFile> mUpdatingDirectory;  // For update only.
   nsCOMPtr<nsIFile> mToDeleteDirectory;
@@ -212,25 +215,27 @@ class Classifier {
 
   // Whether mTableRequestResult is outdated and needs to
   // be reloaded from disk.
   bool mIsTableRequestResultOutdated;
 
   // The copy of mLookupCaches for update only.
   LookupCacheArray mNewLookupCaches;
 
+  // True when Reset() is called.
   bool mUpdateInterrupted;
 
+  // True once CLose() has been called
+  bool mIsClosed;
+
   nsCOMPtr<nsIThread> mUpdateThread;  // For async update.
 
   // Identical to mRootStoreDirectory but for update only because
   // nsIFile is not thread safe and mRootStoreDirectory needs to
   // be accessed in CopyInUseDirForUpdate().
   // It will be initialized right before update on the worker thread.
   nsCOMPtr<nsIFile> mRootStoreDirectoryForUpdate;
-
-  bool mIsClosed;  // true once Close() has been called
 };
 
 }  // namespace safebrowsing
 }  // namespace mozilla
 
 #endif