Bug 1408631 - Release SafeBrowsing lookupcache in worker thread while shutdown. r=francois

MozReview-Commit-ID: HuPUyIDFLPX

--HG--
extra : rebase_source : d6e4f5bbcf96c97541792e23447f0810150c5ac9

toolkit/components/url-classifier/Classifier.cpp

@@ -267,6 +267,8 @@ Classifier::Open(nsIFile& aCacheDirectory)
 void
 Classifier::Close()
 {
+  // Close will be called by PreShutdown, so it is important to note that
+  // things put here should not affect an ongoing update thread.
   DropStores();
 }
 
@@ -1428,10 +1430,8 @@ Classifier::UpdateCache(TableUpdate* aUpdate)
 LookupCache *
 Classifier::GetLookupCache(const nsACString& aTable, bool aForUpdate)
 {
-  if (aForUpdate) {
-    MOZ_ASSERT(NS_GetCurrentThread() == mUpdateThread,
-               "GetLookupCache(aForUpdate==true) can only be called on update thread.");
-  }
+  // GetLookupCache(aForUpdate==true) can only be called on update thread.
+  MOZ_ASSERT_IF(aForUpdate, NS_GetCurrentThread() == mUpdateThread);
 
   nsTArray<LookupCache*>& lookupCaches = aForUpdate ? mNewLookupCaches
                                                     : mLookupCaches;
@@ -1444,6 +1444,11 @@ Classifier::GetLookupCache(const nsACString& aTable, bool aForUpdate)
     }
   }
 
+  // We don't want to create lookupcache when shutdown is already happening.
+  if (nsUrlClassifierDBService::ShutdownHasStarted()) {
+    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'.

toolkit/components/url-classifier/nsUrlClassifierDBService.cpp

@@ -817,6 +817,20 @@ nsUrlClassifierDBServiceWorker::CloseDb()
   return NS_OK;
 }
 
+nsresult
+nsUrlClassifierDBServiceWorker::PreShutdown()
+{
+  if (mClassifier) {
+    // Classifier close will release all lookup caches which may be a time-consuming job.
+    // See Bug 1408631.
+    mClassifier->Close();
+  }
+
+  // WARNING: nothing we put here should affect an ongoing update thread. When in doubt,
+  // put things in Shutdown() instead.
+  return NS_OK;
+}
+
 nsresult
 nsUrlClassifierDBServiceWorker::CacheCompletions(CacheResultArray *results)
 {
@@ -2427,6 +2441,13 @@ nsUrlClassifierDBService::Observe(nsISupports *aSubject, const char *aTopic,
   } else if (!strcmp(aTopic, "quit-application")) {
     // Tell the update thread to finish as soon as possible.
     gShuttingDownThread = true;
+
+    // The code in ::Shutdown() is run on a 'profile-before-change' event and
+    // ensures that objects are freed by blocking on this freeing.
+    // We can however speed up the shutdown time by using the worker thread to
+    // release, in an earlier event, any objects that cannot affect an ongoing
+    // update on the update thread.
+    PreShutdown();
   } else if (!strcmp(aTopic, "profile-before-change")) {
     gShuttingDownThread = true;
     Shutdown();
@@ -2437,6 +2458,22 @@ nsUrlClassifierDBService::Observe(nsISupports *aSubject, const char *aTopic,
   return NS_OK;
 }
 
+// Post a PreShutdown task to worker thread to release objects without blocking
+// main-thread. Notice that shutdown process may still be blocked by PreShutdown task
+// when ::Shutdown() is executed and synchronously waits for worker thread to finish
+// PreShutdown event.
+nsresult
+nsUrlClassifierDBService::PreShutdown()
+{
+  MOZ_ASSERT(XRE_IsParentProcess());
+
+  if (mWorkerProxy) {
+    mWorkerProxy->PreShutdown();
+  }
+
+  return NS_OK;
+}
+
 // Join the background thread if it exists.
 nsresult
 nsUrlClassifierDBService::Shutdown()

toolkit/components/url-classifier/nsUrlClassifierDBService.h

@@ -140,6 +140,9 @@ private:
                      nsIUrlClassifierCallback* c,
                      bool forceCheck, bool *didCheck);
 
+  // Post an event to worker thread to release objects when receive 'quit-application'
+  nsresult PreShutdown();
+
   // Close db connection and join the background thread if it exists.
   nsresult Shutdown();
 
@@ -220,6 +223,8 @@ public:
   // Provide a way to forcibly close the db connection.
   nsresult GCC_MANGLING_WORKAROUND CloseDb();
 
+  nsresult GCC_MANGLING_WORKAROUND PreShutdown();
+
   nsresult CacheCompletions(CacheResultArray * aEntries);
 
   // Used to probe the state of the worker thread. When the update begins,

toolkit/components/url-classifier/nsUrlClassifierProxies.cpp

@@ -214,6 +214,16 @@ UrlClassifierDBServiceWorkerProxy::CloseDb()
   return DispatchToWorkerThread(r);
 }
 
+nsresult
+UrlClassifierDBServiceWorkerProxy::PreShutdown()
+{
+  nsCOMPtr<nsIRunnable> r =
+    NewRunnableMethod("nsUrlClassifierDBServiceWorker::PreShutdown",
+                      mTarget,
+                      &nsUrlClassifierDBServiceWorker::PreShutdown);
+  return DispatchToWorkerThread(r);
+}
+
 nsresult
 UrlClassifierDBServiceWorkerProxy::CacheCompletions(CacheResultArray * aEntries)
 {

toolkit/components/url-classifier/nsUrlClassifierProxies.h

@@ -229,6 +229,7 @@ public:
 
   nsresult OpenDb();
   nsresult CloseDb();
+  nsresult PreShutdown();
 
   nsresult CacheCompletions(mozilla::safebrowsing::CacheResultArray * aEntries);