Bug 1504774 - Fix url-classifier worker thread is not aborted while shutting down. r=francois
authordlee <dlee@mozilla.com>
Wed, 19 Dec 2018 10:03:19 +0000
changeset 451313 5b1c8bfb5cd2f23306f5e6cdbc811789631a50bc
parent 451312 6c607dfcb6272b03df3e8a1f00efa4334fd3361f
child 451314 eddeea139dba1165867c563d0a117b5c0aaaeb18
push id74987
push userdlee@mozilla.com
push dateWed, 19 Dec 2018 14:31:06 +0000
treeherderautoland@5b1c8bfb5cd2 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersfrancois
bugs1504774, 1453038
milestone66.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 1504774 - Fix url-classifier worker thread is not aborted while shutting down. r=francois 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");
 
@@ -758,17 +756,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()));
@@ -779,17 +777,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 {
@@ -809,17 +807,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();
@@ -862,17 +860,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);
 
@@ -1042,30 +1040,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);
 
@@ -1099,16 +1097,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."));
@@ -1190,17 +1191,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())) {
@@ -1292,17 +1293,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;
   }
@@ -1441,17 +1442,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;
@@ -1617,10 +1618,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
@@ -197,16 +197,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;
@@ -220,25 +223,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