From c9e502918ed8adb587a3339bc964f06dbf980ef2 Mon Sep 17 00:00:00 2001 From: dlee Date: Wed, 19 Dec 2018 10:03:19 +0000 Subject: [PATCH] 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 --- .../components/url-classifier/Classifier.cpp | 32 +++++++++++-------- .../components/url-classifier/Classifier.h | 9 ++++-- 2 files changed, 26 insertions(+), 15 deletions(-) diff --git a/toolkit/components/url-classifier/Classifier.cpp b/toolkit/components/url-classifier/Classifier.cpp index 9ce55a3b593d..00caa202ff50 100644 --- a/toolkit/components/url-classifier/Classifier.cpp +++ b/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& aDestDir, @@ -1060,7 +1058,7 @@ nsresult Classifier::CopyDirectoryInterruptible(nsCOMPtr& aDestDir, nsCOMPtr 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& 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 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 diff --git a/toolkit/components/url-classifier/Classifier.h b/toolkit/components/url-classifier/Classifier.h index 6d6fcc348f04..1be929c3bea1 100644 --- a/toolkit/components/url-classifier/Classifier.h +++ b/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 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 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 mRootStoreDirectoryForUpdate; - - bool mIsClosed; // true once Close() has been called }; } // namespace safebrowsing