Bug 1553951 - Safe Browsing doesn't call the lookup callback when an error occurs in DoLocalLookup. r=baku
☠☠ backed out by 88c43064571c ☠ ☠
authordlee <dlee@mozilla.com>
Mon, 01 Jul 2019 11:26:11 +0000
changeset 543610 b308a5b128aa3a34277476fd3237083f5a21e5c2
parent 543609 34b35632f194e4e711d74bce6415dddb34cebf42
child 543611 ca0726ea50cb5e8b02e758b2db7161e472779553
push id2131
push userffxbld-merge
push dateMon, 26 Aug 2019 18:30:20 +0000
treeherdermozilla-release@b19ffb3ca153 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbaku
bugs1553951
milestone69.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1553951 - Safe Browsing doesn't call the lookup callback when an error occurs in DoLocalLookup. r=baku Firefox cannot open any URL when Safe Browsing database is corrupted. This is because when Necko asks URL Classifier to classify an URL, it first suspends the channel, and then wait for a callback to see if the channel should be canceled or resumed. In this bug, DoLocalLookup returns an error because Safe Browsing is not able to read its database(database is corrupted), but we don't call the callback to resume loading channel, so users cannot load any website. This patch adds a scope exit handler to make sure the callback will always be invoked. Differential Revision: https://phabricator.services.mozilla.com/D36456
toolkit/components/url-classifier/nsUrlClassifierDBService.cpp
--- a/toolkit/components/url-classifier/nsUrlClassifierDBService.cpp
+++ b/toolkit/components/url-classifier/nsUrlClassifierDBService.cpp
@@ -369,38 +369,42 @@ nsresult nsUrlClassifierDBServiceWorker:
  * b) If we find any entries, we check the list of fragments for that entry
  *    against the possible subfragments of the URL as described in the
  *    "Simplified Regular Expression Lookup" section of the protocol doc.
  */
 nsresult nsUrlClassifierDBServiceWorker::DoLookup(
     const nsACString& spec,
     nsUrlClassifierDBService::FeatureHolder* aFeatureHolder,
     nsIUrlClassifierLookupCallback* c) {
+  // Make sure the callback is invoked when a failure occurs,
+  // otherwise we will not be able to load any url.
+  auto scopeExit = MakeScopeExit([c]() { c->LookupComplete(nullptr); });
+
   if (gShuttingDownThread) {
-    c->LookupComplete(nullptr);
     return NS_ERROR_NOT_INITIALIZED;
   }
 
   PRIntervalTime clockStart = 0;
   if (LOG_ENABLED()) {
     clockStart = PR_IntervalNow();
   }
 
   nsresult rv = aFeatureHolder->DoLocalLookup(spec, this);
-  NS_ENSURE_SUCCESS(rv, rv);
+  if (NS_WARN_IF(NS_FAILED(rv))) {
+    return rv;
+  }
 
   if (LOG_ENABLED()) {
     PRIntervalTime clockEnd = PR_IntervalNow();
     LOG(("query took %dms\n",
          PR_IntervalToMilliseconds(clockEnd - clockStart)));
   }
 
   UniquePtr<LookupResultArray> results = aFeatureHolder->GetTableResults();
   if (NS_WARN_IF(!results)) {
-    c->LookupComplete(nullptr);
     return NS_ERROR_OUT_OF_MEMORY;
   }
 
   LOG(("Found %zu results.", results->Length()));
 
   for (const RefPtr<const LookupResult> lookupResult : *results) {
     if (!lookupResult->Confirmed() &&
         mDBService->CanComplete(lookupResult->mTableName)) {
@@ -409,16 +413,17 @@ nsresult nsUrlClassifierDBServiceWorker:
       // add to completes, which can cause completes to reallocate and move.
       AddNoise(lookupResult->hash.fixedLengthPrefix, lookupResult->mTableName,
                mGethashNoise, *results);
       break;
     }
   }
 
   // At this point ownership of 'results' is handed to the callback.
+  scopeExit.release();
   c->LookupComplete(std::move(results));
 
   return NS_OK;
 }
 
 nsresult nsUrlClassifierDBServiceWorker::HandlePendingLookups() {
   if (gShuttingDownThread) {
     return NS_ERROR_ABORT;