Bug 1408396 - Fix a deadlock issue when about:url-classifier accesses GetCacheInfo API. r?francois draft
authorDimiL <dlee@mozilla.com>
Wed, 18 Oct 2017 09:11:26 +0800
changeset 682002 5a80ced1d7c1d6a17d8da740d534e183716d1371
parent 681956 f78d5947333422ab09ec23e3dab0d48538c9d6ad
child 736287 bd1fc9964d85155d2797a2c4b3117221e7524ee5
push id84987
push userbmo:dlee@mozilla.com
push dateWed, 18 Oct 2017 01:43:47 +0000
reviewersfrancois
bugs1408396
milestone58.0a1
Bug 1408396 - Fix a deadlock issue when about:url-classifier accesses GetCacheInfo API. r?francois When about:url-classifier ask cache information by using |GetCacheInfo| API, this API will send a SYNC request to off-main thread. If NSS module is not initialized then off-main thread will send init request to main thread, and this causes a deadlock. This patch change |GetCacheInfo| API to asynchronous so main thread will not be blocked. MozReview-Commit-ID: 3y2hOjCoRUj
toolkit/components/url-classifier/nsIUrlClassifierInfo.idl
toolkit/components/url-classifier/nsUrlClassifierDBService.cpp
toolkit/components/url-classifier/nsUrlClassifierProxies.cpp
toolkit/components/url-classifier/nsUrlClassifierProxies.h
toolkit/content/aboutUrlClassifier.js
--- a/toolkit/components/url-classifier/nsIUrlClassifierInfo.idl
+++ b/toolkit/components/url-classifier/nsIUrlClassifierInfo.idl
@@ -56,18 +56,25 @@ interface nsIUrlClassifierCacheInfo : ns
   readonly attribute ACString table;
 
   /*
    * An array of nsIUrlClassifierCacheEntry.
    */
   readonly attribute nsIArray entries;
 };
 
+[scriptable, uuid(26e12ea4-14ff-4c77-858f-6745998b7659)]
+interface nsIUrlClassifierGetCacheCallback : nsISupports {
+
+  void onGetCacheComplete(in nsIUrlClassifierCacheInfo info);
+};
+
 /**
  * Interface to query url-classifier information.
  */
 [scriptable, function, uuid(411bbff4-1b88-4687-aa36-e2bbdd93f6e8)]
 interface nsIUrlClassifierInfo : nsISupports {
   /**
-   * A synchronous call to return cache information for the table.
+   * An asynchronous call to return cache information for the table.
    */
-  nsIUrlClassifierCacheInfo getCacheInfo(in ACString table);
+  void getCacheInfo(in ACString table,
+                    in nsIUrlClassifierGetCacheCallback callback);
 };
--- a/toolkit/components/url-classifier/nsUrlClassifierDBService.cpp
+++ b/toolkit/components/url-classifier/nsUrlClassifierDBService.cpp
@@ -2364,21 +2364,22 @@ nsUrlClassifierDBService::ClearCache()
   NS_ENSURE_TRUE(gDbBackgroundThread, NS_ERROR_NOT_INITIALIZED);
 
   return mWorkerProxy->ClearCache();
 }
 
 
 NS_IMETHODIMP
 nsUrlClassifierDBService::GetCacheInfo(const nsACString& aTable,
-                                       nsIUrlClassifierCacheInfo** aCache)
+                                       nsIUrlClassifierGetCacheCallback* aCallback)
 {
   NS_ENSURE_TRUE(gDbBackgroundThread, NS_ERROR_NOT_INITIALIZED);
 
-  return mWorkerProxy->GetCacheInfo(aTable, aCache);
+  return mWorkerProxy->GetCacheInfo(aTable, aCallback);
+  return NS_OK;
 }
 
 nsresult
 nsUrlClassifierDBService::CacheCompletions(CacheResultArray *results)
 {
   NS_ENSURE_TRUE(gDbBackgroundThread, NS_ERROR_NOT_INITIALIZED);
 
   return mWorkerProxy->CacheCompletions(results);
--- a/toolkit/components/url-classifier/nsUrlClassifierProxies.cpp
+++ b/toolkit/components/url-classifier/nsUrlClassifierProxies.cpp
@@ -238,35 +238,44 @@ UrlClassifierDBServiceWorkerProxy::Clear
 NS_IMETHODIMP
 UrlClassifierDBServiceWorkerProxy::ClearLastResultsRunnable::Run()
 {
   return mTarget->ClearLastResults();
 }
 
 nsresult
 UrlClassifierDBServiceWorkerProxy::GetCacheInfo(const nsACString& aTable,
-                                                nsIUrlClassifierCacheInfo** aCache)
+                                                nsIUrlClassifierGetCacheCallback* aCallback)
 {
-  nsCOMPtr<nsIRunnable> r = new GetCacheInfoRunnable(mTarget, aTable, aCache);
-
-  nsIThread* t = nsUrlClassifierDBService::BackgroundThread();
-  if (!t) {
-    return NS_ERROR_FAILURE;
-  }
-
-  // This blocks main thread but since 'GetCacheInfo' is only used by
-  // about:url-classifier so it should be fine.
-  mozilla::SyncRunnable::DispatchToThread(t, r);
-  return NS_OK;
+  nsCOMPtr<nsIRunnable> r = new GetCacheInfoRunnable(mTarget, aTable, aCallback);
+  return DispatchToWorkerThread(r);
 }
 
 NS_IMETHODIMP
 UrlClassifierDBServiceWorkerProxy::GetCacheInfoRunnable::Run()
 {
-  return mTarget->GetCacheInfo(mTable, mCache);
+  MOZ_ASSERT(mCallback);
+
+  mTarget->GetCacheInfo(mTable, &mCache);
+
+  nsCOMPtr<nsIRunnable> r = new GetCacheInfoCallbackRunnable(mCache, mCallback);
+  return NS_DispatchToMainThread(r);
+}
+
+
+NS_IMETHODIMP
+UrlClassifierDBServiceWorkerProxy::GetCacheInfoCallbackRunnable::Run()
+{
+  MOZ_ASSERT(NS_IsMainThread(), "Must be called on main thread");
+  MOZ_ASSERT(mCallback);
+
+  mCallback->OnGetCacheComplete(mCache);
+  NS_RELEASE(mCache);
+
+  return NS_OK;
 }
 
 NS_IMPL_ISUPPORTS(UrlClassifierLookupCallbackProxy,
                   nsIUrlClassifierLookupCallback)
 
 NS_IMETHODIMP
 UrlClassifierLookupCallbackProxy::LookupComplete
   (LookupResultArray * aResults)
--- a/toolkit/components/url-classifier/nsUrlClassifierProxies.h
+++ b/toolkit/components/url-classifier/nsUrlClassifierProxies.h
@@ -182,43 +182,63 @@ public:
     RefPtr<nsUrlClassifierDBServiceWorker> mTarget;
   };
 
   class GetCacheInfoRunnable: public mozilla::Runnable
   {
   public:
     explicit GetCacheInfoRunnable(nsUrlClassifierDBServiceWorker* aTarget,
                                   const nsACString& aTable,
-                                  nsIUrlClassifierCacheInfo** aCache)
+                                  nsIUrlClassifierGetCacheCallback* aCallback)
       : mozilla::Runnable(
           "UrlClassifierDBServiceWorkerProxy::GetCacheInfoRunnable")
       , mTarget(aTarget)
       , mTable(aTable)
-      , mCache(aCache)
+      , mCache(nullptr)
+      , mCallback(new nsMainThreadPtrHolder<nsIUrlClassifierGetCacheCallback>(
+          "nsIUrlClassifierGetCacheCallback", aCallback))
     { }
 
     NS_DECL_NSIRUNNABLE
   private:
     RefPtr<nsUrlClassifierDBServiceWorker> mTarget;
     nsCString mTable;
-    nsIUrlClassifierCacheInfo** mCache;
+    nsIUrlClassifierCacheInfo* mCache;
+    nsMainThreadPtrHandle<nsIUrlClassifierGetCacheCallback> mCallback;
+  };
+
+  class GetCacheInfoCallbackRunnable: public mozilla::Runnable
+  {
+  public:
+    explicit GetCacheInfoCallbackRunnable(nsIUrlClassifierCacheInfo* aCache,
+                                          nsMainThreadPtrHandle<nsIUrlClassifierGetCacheCallback>& aCallback)
+      : mozilla::Runnable(
+          "UrlClassifierDBServiceWorkerProxy::GetCacheInfoCallbackRunnable")
+      , mCache(aCache)
+      , mCallback(aCallback)
+    { }
+
+    NS_DECL_NSIRUNNABLE
+  private:
+    nsIUrlClassifierCacheInfo* mCache;
+    nsMainThreadPtrHandle<nsIUrlClassifierGetCacheCallback> mCallback;
   };
 
 public:
   nsresult DoLocalLookup(const nsACString& spec,
                          const nsACString& tables,
                          mozilla::safebrowsing::LookupResultArray* results);
 
   nsresult OpenDb();
   nsresult CloseDb();
 
   nsresult CacheCompletions(mozilla::safebrowsing::CacheResultArray * aEntries);
 
   nsresult GetCacheInfo(const nsACString& aTable,
-                        nsIUrlClassifierCacheInfo** aCache);
+                        nsIUrlClassifierGetCacheCallback* aCallback);
 private:
   ~UrlClassifierDBServiceWorkerProxy() {}
 
   RefPtr<nsUrlClassifierDBServiceWorker> mTarget;
 };
 
 // The remaining classes here are all proxies to the main thread
 
--- a/toolkit/content/aboutUrlClassifier.js
+++ b/toolkit/content/aboutUrlClassifier.js
@@ -262,67 +262,70 @@ var Cache = {
     let dbservice = Cc["@mozilla.org/url-classifier/dbservice;1"]
                     .getService(Ci.nsIUrlClassifierInfo);
 
     for (let provider of Provider.providers) {
       let pref = "browser.safebrowsing.provider." + provider + ".lists";
       let tables = Services.prefs.getCharPref(pref, "").split(",");
 
       for (let table of tables) {
-        let cache = dbservice.getCacheInfo(table);
-        let entries = cache.entries;
-        if (entries.length === 0) {
-          this.showCacheEnties.delete(table);
-          continue;
-        }
+        dbservice.getCacheInfo(table, {
+          onGetCacheComplete: (aCache) => {
+            let entries = aCache.entries;
+            if (entries.length === 0) {
+              this.showCacheEnties.delete(table);
+              return;
+            }
 
-        let positiveCacheCount = 0;
-        for (let i = 0; i < entries.length ; i++) {
-          let entry = entries.queryElementAt(i, Ci.nsIUrlClassifierCacheEntry);
-          let matches = entry.matches;
-          positiveCacheCount += matches.length;
+            let positiveCacheCount = 0;
+            for (let i = 0; i < entries.length ; i++) {
+              let entry = entries.queryElementAt(i, Ci.nsIUrlClassifierCacheEntry);
+              let matches = entry.matches;
+              positiveCacheCount += matches.length;
 
-          // If we don't have to show cache entries for this table then just
-          // skip the following code.
-          if (!this.showCacheEnties.has(table)) {
-            continue;
-          }
+              // If we don't have to show cache entries for this table then just
+              // skip the following code.
+              if (!this.showCacheEnties.has(table)) {
+                continue;
+              }
 
-          let tds = [table, entry.prefix, new Date(entry.expiry * 1000).toString()];
-          let j = 0;
-          do {
-            if (matches.length >= 1) {
-              let match =
-                matches.queryElementAt(j, Ci.nsIUrlClassifierPositiveCacheEntry);
-              let list = [match.fullhash, new Date(match.expiry * 1000).toString()];
-              tds = tds.concat(list);
-            } else {
-              tds = tds.concat([STR_NA, STR_NA]);
+              let tds = [table, entry.prefix, new Date(entry.expiry * 1000).toString()];
+              let j = 0;
+              do {
+                if (matches.length >= 1) {
+                  let match =
+                    matches.queryElementAt(j, Ci.nsIUrlClassifierPositiveCacheEntry);
+                  let list = [match.fullhash, new Date(match.expiry * 1000).toString()];
+                  tds = tds.concat(list);
+                } else {
+                  tds = tds.concat([STR_NA, STR_NA]);
+                }
+                createRow(tds, document.getElementById("cache-entries-table-body"), 5);
+                j++;
+                tds = [""];
+              } while (j < matches.length);
             }
-            createRow(tds, document.getElementById("cache-entries-table-body"), 5);
-            j++;
-            tds = [""];
-          } while (j < matches.length);
-        }
 
-        // Create cache information entries.
-        let chk = document.createElement("input");
-        chk.type = "checkbox";
-        chk.checked = this.showCacheEnties.has(table);
-        chk.addEventListener("click", () => {
-          if (chk.checked) {
-            this.showCacheEnties.add(table);
-          } else {
-            this.showCacheEnties.delete(table);
+            // Create cache information entries.
+            let chk = document.createElement("input");
+            chk.type = "checkbox";
+            chk.checked = this.showCacheEnties.has(table);
+            chk.addEventListener("click", () => {
+              if (chk.checked) {
+                this.showCacheEnties.add(table);
+              } else {
+                this.showCacheEnties.delete(table);
+              }
+              this.refresh();
+            });
+
+            let tds = [table, entries.length, positiveCacheCount, chk];
+            createRow(tds, document.getElementById("cache-table-body"), tds.length);
           }
-          this.refresh();
         });
-
-        let tds = [table, entries.length, positiveCacheCount, chk];
-        createRow(tds, document.getElementById("cache-table-body"), tds.length);
       }
     }
 
     let entries_div = document.getElementById("cache-entries");
     entries_div.style.display = this.showCacheEnties.size == 0 ? "none" : "block";
   },
 };