Bug 1315386 - Make Safe Browsing code more shutdown-aware. r=francois,gcp.

MozReview-Commit-ID: ATCVfh5YLZl

--HG--
extra : rebase_source : d87cfae3838f1a26dc67d2546264e47768011a63

toolkit/components/downloads/ApplicationReputation.cpp

@@ -92,13 +92,15 @@ class PendingDBLookup;
 // created by ApplicationReputationService, it is guaranteed to call mCallback.
 // This class is private to ApplicationReputationService.
 class PendingLookup final : public nsIStreamListener,
-                            public nsITimerCallback
+                            public nsITimerCallback,
+                            public nsIObserver
 {
 public:
   NS_DECL_ISUPPORTS
   NS_DECL_NSIREQUESTOBSERVER
   NS_DECL_NSISTREAMLISTENER
   NS_DECL_NSITIMERCALLBACK
+  NS_DECL_NSIOBSERVER
 
   // Constructor and destructor.
   PendingLookup(nsIApplicationReputationQuery* aQuery,
@@ -370,7 +372,8 @@ PendingDBLookup::HandleEvent(const nsACString& tables)
 
 NS_IMPL_ISUPPORTS(PendingLookup,
                   nsIStreamListener,
-                  nsIRequestObserver)
+                  nsIRequestObserver,
+                  nsIObserver)
 
 PendingLookup::PendingLookup(nsIApplicationReputationQuery* aQuery,
                              nsIApplicationReputationCallback* aCallback) :
@@ -1322,6 +1325,24 @@ PendingLookup::Notify(nsITimer* aTimer)
   return NS_OK;
 }
 
+///////////////////////////////////////////////////////////////////////////////
+// nsIObserver implementation
+NS_IMETHODIMP
+PendingLookup::Observe(nsISupports *aSubject, const char *aTopic,
+                       const char16_t *aData)
+{
+  if (!strcmp(aTopic, "quit-application")) {
+    if (mTimeoutTimer) {
+      mTimeoutTimer->Cancel();
+      mTimeoutTimer = nullptr;
+    }
+    if (mChannel) {
+      mChannel->Cancel(NS_ERROR_ABORT);
+    }
+  }
+  return NS_OK;
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 //// nsIStreamListener
 static nsresult
@@ -1519,5 +1540,13 @@ nsresult ApplicationReputationService::QueryReputationInternal(
   RefPtr<PendingLookup> lookup(new PendingLookup(aQuery, aCallback));
   NS_ENSURE_STATE(lookup);
 
+  // Add an observer for shutdown
+  nsCOMPtr<nsIObserverService> observerService =
+    mozilla::services::GetObserverService();
+  if (!observerService) {
+    return NS_ERROR_FAILURE;
+  }
+
+  observerService->AddObserver(lookup, "quit-application", false);
   return lookup->StartLookup();
 }

toolkit/components/url-classifier/Classifier.cpp

@@ -336,6 +336,11 @@ Classifier::DeleteTables(nsIFile* aDirectory, const nsTArray<nsCString>& aTables
 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.
@@ -946,6 +951,10 @@ nsresult
 Classifier::UpdateHashStore(nsTArray<TableUpdate*>* aUpdates,
                             const nsACString& aTable)
 {
+  if (nsUrlClassifierDBService::ShutdownHasStarted()) {
+    return NS_ERROR_ABORT;
+  }
+
   LOG(("Classifier::UpdateHashStore(%s)", PromiseFlatCString(aTable).get()));
 
   HashStore store(aTable, GetProvider(aTable), mRootStoreDirectory);
@@ -1044,6 +1053,9 @@ Classifier::UpdateTableV4(nsTArray<TableUpdate*>* aUpdates,
 {
   MOZ_ASSERT(!NS_IsMainThread(),
              "UpdateTableV4 must be called on the classifier worker thread.");
+  if (nsUrlClassifierDBService::ShutdownHasStarted()) {
+    return NS_ERROR_ABORT;
+  }
 
   LOG(("Classifier::UpdateTableV4(%s)", PromiseFlatCString(aTable).get()));
 

toolkit/components/url-classifier/HashStore.cpp

@@ -39,6 +39,7 @@
 #include "mozilla/Logging.h"
 #include "zlib.h"
 #include "Classifier.h"
+#include "nsUrlClassifierDBService.h"
 
 // Main store for SafeBrowsing protocol data. We store
 // known add/sub chunks, prefixes and completions in memory
@@ -951,6 +952,9 @@ nsresult
 HashStore::WriteFile()
 {
   NS_ASSERTION(mInUpdate, "Must be in update to write database.");
+  if (nsUrlClassifierDBService::ShutdownHasStarted()) {
+    return NS_ERROR_ABORT;
+  }
 
   nsCOMPtr<nsIFile> storeFile;
   nsresult rv = mStoreDirectory->Clone(getter_AddRefs(storeFile));

toolkit/components/url-classifier/LookupCache.cpp

@@ -144,6 +144,10 @@ LookupCache::DumpCache()
 nsresult
 LookupCache::WriteFile()
 {
+  if (nsUrlClassifierDBService::ShutdownHasStarted()) {
+    return NS_ERROR_ABORT;
+  }
+
   nsCOMPtr<nsIFile> psFile;
   nsresult rv = mStoreDirectory->Clone(getter_AddRefs(psFile));
   NS_ENSURE_SUCCESS(rv, rv);

toolkit/components/url-classifier/LookupCacheV4.cpp

@@ -447,6 +447,9 @@ nsresult
 LookupCacheV4::WriteMetadata(TableUpdateV4* aTableUpdate)
 {
   NS_ENSURE_ARG_POINTER(aTableUpdate);
+  if (nsUrlClassifierDBService::ShutdownHasStarted()) {
+    return NS_ERROR_ABORT;
+  }
 
   nsCOMPtr<nsIFile> metaFile;
   nsresult rv = mStoreDirectory->Clone(getter_AddRefs(metaFile));

toolkit/components/url-classifier/ProtocolParser.cpp

@@ -10,6 +10,7 @@
 #include "prnetdb.h"
 #include "prprf.h"
 
+#include "nsUrlClassifierDBService.h"
 #include "nsUrlClassifierUtils.h"
 #include "nsPrintfCString.h"
 #include "mozilla/Base64.h"
@@ -144,6 +145,10 @@ ProtocolParserV2::AppendStream(const nsACString& aData)
 
   bool done = false;
   while (!done) {
+    if (nsUrlClassifierDBService::ShutdownHasStarted()) {
+      return NS_ERROR_ABORT;
+    }
+
     if (mState == PROTOCOL_STATE_CONTROL) {
       rv = ProcessControl(&done);
     } else if (mState == PROTOCOL_STATE_CHUNK) {

toolkit/components/url-classifier/nsUrlClassifierDBService.cpp

@@ -193,6 +193,10 @@ nsUrlClassifierDBServiceWorker::DoLocalLookup(const nsACString& spec,
                                               const nsACString& tables,
                                               LookupResultArray* results)
 {
+  if (gShuttingDownThread) {
+    return NS_ERROR_ABORT;
+  }
+
   MOZ_ASSERT(!NS_IsMainThread(), "DoLocalLookup must be on background thread");
   if (!results) {
     return NS_ERROR_FAILURE;
@@ -308,6 +312,10 @@ nsUrlClassifierDBServiceWorker::DoLookup(const nsACString& spec,
 nsresult
 nsUrlClassifierDBServiceWorker::HandlePendingLookups()
 {
+  if (gShuttingDownThread) {
+    return NS_ERROR_ABORT;
+  }
+
   MutexAutoLock lock(mPendingLookupLock);
   while (mPendingLookups.Length() > 0) {
     PendingLookup lookup = mPendingLookups[0];
@@ -330,6 +338,10 @@ nsUrlClassifierDBServiceWorker::AddNoise(const Prefix aPrefix,
                                          uint32_t aCount,
                                          LookupResultArray& results)
 {
+  if (gShuttingDownThread) {
+    return NS_ERROR_ABORT;
+  }
+
   if (aCount < 1) {
     return NS_OK;
   }
@@ -359,14 +371,19 @@ nsUrlClassifierDBServiceWorker::Lookup(nsIPrincipal* aPrincipal,
                                        const nsACString& aTables,
                                        nsIUrlClassifierCallback* c)
 {
+  if (gShuttingDownThread) {
+    return NS_ERROR_ABORT;
+  }
+
   return HandlePendingLookups();
 }
 
 NS_IMETHODIMP
 nsUrlClassifierDBServiceWorker::GetTables(nsIUrlClassifierCallback* c)
 {
-  if (gShuttingDownThread)
+  if (gShuttingDownThread) {
     return NS_ERROR_NOT_INITIALIZED;
+  }
 
   nsresult rv = OpenDb();
   if (NS_FAILED(rv)) {
@@ -440,8 +457,9 @@ nsUrlClassifierDBServiceWorker::BeginStream(const nsACString &table)
   LOG(("nsUrlClassifierDBServiceWorker::BeginStream"));
   MOZ_ASSERT(!NS_IsMainThread(), "Streaming must be on the background thread");
 
-  if (gShuttingDownThread)
+  if (gShuttingDownThread) {
     return NS_ERROR_NOT_INITIALIZED;
+  }
 
   NS_ENSURE_STATE(mUpdateObserver);
   NS_ENSURE_STATE(!mInStream);
@@ -520,8 +538,9 @@ nsUrlClassifierDBServiceWorker::BeginStream(const nsACString &table)
 NS_IMETHODIMP
 nsUrlClassifierDBServiceWorker::UpdateStream(const nsACString& chunk)
 {
-  if (gShuttingDownThread)
+  if (gShuttingDownThread) {
     return NS_ERROR_NOT_INITIALIZED;
+  }
 
   NS_ENSURE_STATE(mInStream);
 
@@ -587,8 +606,10 @@ nsUrlClassifierDBServiceWorker::FinishStream()
 NS_IMETHODIMP
 nsUrlClassifierDBServiceWorker::FinishUpdate()
 {
-  if (gShuttingDownThread)
+  if (gShuttingDownThread) {
     return NS_ERROR_NOT_INITIALIZED;
+  }
+
   NS_ENSURE_STATE(mUpdateObserver);
 
   if (NS_SUCCEEDED(mUpdateStatus)) {
@@ -739,6 +760,10 @@ nsUrlClassifierDBServiceWorker::CloseDb()
 nsresult
 nsUrlClassifierDBServiceWorker::CacheCompletions(CacheResultArray *results)
 {
+  if (gShuttingDownThread) {
+    return NS_ERROR_ABORT;
+  }
+
   LOG(("nsUrlClassifierDBServiceWorker::CacheCompletions [%p]", this));
   if (!mClassifier)
     return NS_OK;
@@ -818,6 +843,10 @@ nsUrlClassifierDBServiceWorker::CacheMisses(PrefixArray *results)
 nsresult
 nsUrlClassifierDBServiceWorker::OpenDb()
 {
+  if (gShuttingDownThread) {
+    return NS_ERROR_ABORT;
+  }
+
   MOZ_ASSERT(!NS_IsMainThread(), "Must initialize DB on background thread");
   // Connection already open, don't do anything.
   if (mClassifier) {
@@ -1347,8 +1376,9 @@ nsUrlClassifierDBService::Init()
   if (!observerService)
     return NS_ERROR_FAILURE;
 
+  // The application is about to quit
+  observerService->AddObserver(this, "quit-application", false);
   observerService->AddObserver(this, "profile-before-change", false);
-  observerService->AddObserver(this, "xpcom-shutdown-threads", false);
 
   // XXX: Do we *really* need to be able to change all of these at runtime?
   // Note: These observers should only be added when everything else above has
@@ -1468,6 +1498,9 @@ nsUrlClassifierDBService::ClassifyLocalWithTables(nsIURI *aURI,
                                                   nsACString & aTableResults)
 {
   MOZ_ASSERT(NS_IsMainThread(), "ClassifyLocalWithTables must be on main thread");
+  if (gShuttingDownThread) {
+    return NS_ERROR_ABORT;
+  }
 
   if (XRE_IsContentProcess()) {
     using namespace mozilla::dom;
@@ -1798,9 +1831,18 @@ nsUrlClassifierDBService::Observe(nsISupports *aSubject, const char *aTopic,
       gFreshnessGuarantee = Preferences::GetInt(CONFIRM_AGE_PREF,
         CONFIRM_AGE_DEFAULT_SEC);
     }
-  } else if (!strcmp(aTopic, "profile-before-change") ||
-             !strcmp(aTopic, "xpcom-shutdown-threads")) {
+  } else if (!strcmp(aTopic, "quit-application")) {
     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;
+    nsIThread *backgroundThread = gDbBackgroundThread;
+    gDbBackgroundThread = nullptr;
+    backgroundThread->Shutdown();
+    NS_RELEASE(backgroundThread);
   } else {
     return NS_ERROR_UNEXPECTED;
   }
@@ -1815,9 +1857,11 @@ nsUrlClassifierDBService::Shutdown()
   LOG(("shutting down db service\n"));
   MOZ_ASSERT(XRE_IsParentProcess());
 
-  if (!gDbBackgroundThread)
+  if (!gDbBackgroundThread || gShuttingDownThread)
     return NS_OK;
 
+  gShuttingDownThread = true;
+
   Telemetry::AutoTimer<Telemetry::URLCLASSIFIER_SHUTDOWN_TIME> timer;
 
   mCompleters.Clear();
@@ -1849,18 +1893,6 @@ nsUrlClassifierDBService::Shutdown()
     rv = mWorkerProxy->CloseDb();
     NS_ASSERTION(NS_SUCCEEDED(rv), "failed to post close db event");
   }
-
-  mWorkerProxy = nullptr;
-
-  LOG(("joining background thread"));
-
-  gShuttingDownThread = true;
-
-  nsIThread *backgroundThread = gDbBackgroundThread;
-  gDbBackgroundThread = nullptr;
-  backgroundThread->Shutdown();
-  NS_RELEASE(backgroundThread);
-
   return NS_OK;
 }
 
@@ -1870,3 +1902,9 @@ nsUrlClassifierDBService::BackgroundThread()
   return gDbBackgroundThread;
 }
 
+// static
+bool
+nsUrlClassifierDBService::ShutdownHasStarted()
+{
+  return gShuttingDownThread;
+}

toolkit/components/url-classifier/nsUrlClassifierDBService.h

@@ -80,6 +80,8 @@ public:
 
   static nsIThread* BackgroundThread();
 
+  static bool ShutdownHasStarted();
+
 private:
   // No subclassing
   ~nsUrlClassifierDBService();

toolkit/components/url-classifier/nsUrlClassifierHashCompleter.js

@@ -155,10 +155,11 @@ function HashCompleter() {
   // A map of gethash URLs to RequestBackoff objects.
   this._backoffs = {};
 
-  // Whether we have been informed of a shutdown by the xpcom-shutdown event.
+  // Whether we have been informed of a shutdown by the shutdown event.
   this._shuttingDown = false;
 
-  Services.obs.addObserver(this, "xpcom-shutdown", true);
+  Services.obs.addObserver(this, "quit-application", false);
+
 }
 
 HashCompleter.prototype = {
@@ -259,8 +260,9 @@ HashCompleter.prototype = {
   },
 
   observe: function HC_observe(aSubject, aTopic, aData) {
-    if (aTopic == "xpcom-shutdown") {
+    if (aTopic == "quit-application") {
       this._shuttingDown = true;
+      Services.obs.removeObserver(this, "quit-application");
     }
   },
 };
@@ -274,7 +276,7 @@ function HashCompleterRequest(aCompleter, aGethashUrl) {
   this._channel = null;
   // Response body of hash completion. Created in onDataAvailable.
   this._response = "";
-  // Whether we have been informed of a shutdown by the xpcom-shutdown event.
+  // Whether we have been informed of a shutdown by the quit-application event.
   this._shuttingDown = false;
   this.gethashUrl = aGethashUrl;
 }
@@ -304,7 +306,7 @@ HashCompleterRequest.prototype = {
       return;
     }
 
-    Services.obs.addObserver(this, "xpcom-shutdown", false);
+    Services.obs.addObserver(this, "quit-application", false);
 
     try {
       this.openChannel();
@@ -512,7 +514,7 @@ HashCompleterRequest.prototype = {
   },
 
   onStopRequest: function HCR_onStopRequest(aRequest, aContext, aStatusCode) {
-    Services.obs.removeObserver(this, "xpcom-shutdown");
+    Services.obs.removeObserver(this, "quit-application");
 
     if (this._shuttingDown) {
       throw Cr.NS_ERROR_ABORT;
@@ -559,13 +561,13 @@ HashCompleterRequest.prototype = {
   },
 
   observe: function HCR_observe(aSubject, aTopic, aData) {
-    if (aTopic != "xpcom-shutdown") {
-      return;
-    }
+    if (aTopic == "quit-application") {
+      this._shuttingDown = true;
+      if (this._channel) {
+        this._channel.cancel(Cr.NS_ERROR_ABORT);
+      }
 
-    this._shuttingDown = true;
-    if (this._channel) {
-      this._channel.cancel(Cr.NS_ERROR_ABORT);
+      Services.obs.removeObserver(this, "quit-application");
     }
   },
 };