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

--HG--
extra : moz-landing-system : lando

toolkit/components/url-classifier/Classifier.cpp

@@ -174,7 +174,7 @@ nsresult Classifier::SetupPathNames() {
 }
 
 nsresult Classifier::CreateStoreDirectory() {
-  if (mIsClosed) {
+  if (ShouldAbort()) {
     return NS_OK;  // nothing to do, the classifier is done
   }
 
@@ -236,10 +236,8 @@ nsresult Classifier::Open(nsIFile& aCacheDirectory) {
 }
 
 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();
 }
@@ -763,7 +761,7 @@ nsresult Classifier::ApplyUpdatesBackground(TableUpdateArray& aUpdates,
 
   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)) {
@@ -784,7 +782,7 @@ nsresult Classifier::ApplyUpdatesBackground(TableUpdateArray& aUpdates,
     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;
     }
@@ -814,7 +812,7 @@ nsresult Classifier::ApplyUpdatesBackground(TableUpdateArray& aUpdates,
 
 nsresult Classifier::ApplyUpdatesForeground(
     nsresult aBackgroundRv, const nsACString& aFailedTableName) {
-  if (mUpdateInterrupted) {
+  if (ShouldAbort()) {
     LOG(("Update is interrupted! Just remove update intermediaries."));
     RemoveUpdateIntermediaries();
     return NS_OK;
@@ -867,7 +865,7 @@ void Classifier::DropStores() {
 }
 
 nsresult Classifier::RegenActiveTables() {
-  if (mIsClosed) {
+  if (ShouldAbort()) {
     return NS_OK;  // nothing to do, the classifier is done
   }
 
@@ -1047,7 +1045,7 @@ nsresult Classifier::DumpFailedUpdate() {
 
 /**
  * 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,
@@ -1060,7 +1058,7 @@ nsresult Classifier::CopyDirectoryInterruptible(nsCOMPtr<nsIFile>& aDestDir,
   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;
     }
@@ -1104,6 +1102,9 @@ nsresult Classifier::CopyDirectoryInterruptible(nsCOMPtr<nsIFile>& aDestDir,
 
 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.
@@ -1195,7 +1196,7 @@ nsCString Classifier::GetProvider(const nsACString& aTableName) {
  */
 nsresult Classifier::UpdateHashStore(TableUpdateArray& aUpdates,
                                      const nsACString& aTable) {
-  if (nsUrlClassifierDBService::ShutdownHasStarted()) {
+  if (ShouldAbort()) {
     return NS_ERROR_UC_UPDATE_SHUTDOWNING;
   }
 
@@ -1297,7 +1298,7 @@ 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;
   }
 
@@ -1446,7 +1447,7 @@ RefPtr<LookupCache> Classifier::GetLookupCache(const nsACString& aTable,
   }
 
   // We don't want to create lookupcache when shutdown is already happening.
-  if (nsUrlClassifierDBService::ShutdownHasStarted()) {
+  if (ShouldAbort()) {
     return nullptr;
   }
 
@@ -1622,5 +1623,10 @@ nsresult Classifier::LoadMetadata(nsIFile* aDirectory, nsACString& aResult) {
   return rv;
 }
 
+bool Classifier::ShouldAbort() const {
+  return mIsClosed || nsUrlClassifierDBService::ShutdownHasStarted() ||
+         (mUpdateInterrupted && (NS_GetCurrentThread() == mUpdateThread));
+}
+
 }  // namespace safebrowsing
 }  // namespace mozilla

toolkit/components/url-classifier/Classifier.h

@@ -202,6 +202,9 @@ class Classifier {
   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.
@@ -225,8 +228,12 @@ class Classifier {
   // 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
@@ -234,8 +241,6 @@ class Classifier {
   // 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