Bug 1122691: Skip ClassifyLocal unless tracking protection is enabled (r=mcmanus,gcp)

netwerk/base/nsBaseChannel.cpp

@@ -292,7 +292,7 @@ nsBaseChannel::ClassifyURI()
   if (mLoadFlags & LOAD_CLASSIFY_URI) {
     nsRefPtr<nsChannelClassifier> classifier = new nsChannelClassifier();
     if (classifier) {
-      classifier->Start(this);
+      classifier->Start(this, false);
     } else {
       Cancel(NS_ERROR_OUT_OF_MEMORY);
     }

netwerk/base/nsChannelClassifier.cpp

@@ -211,10 +211,14 @@ nsChannelClassifier::NotifyTrackingProtectionDisabled(nsIChannel *aChannel)
 }
 
 void
-nsChannelClassifier::Start(nsIChannel *aChannel)
+nsChannelClassifier::Start(nsIChannel *aChannel, bool aContinueBeginConnect)
 {
   mChannel = aChannel;
-  nsresult rv = StartInternal(aChannel);
+  if (aContinueBeginConnect) {
+    mChannelInternal = do_QueryInterface(aChannel);
+  }
+
+  nsresult rv = StartInternal();
   if (NS_FAILED(rv)) {
     // If we aren't getting a callback for any reason, assume a good verdict and
     // make sure we resume the channel if necessary.
@@ -223,7 +227,7 @@ nsChannelClassifier::Start(nsIChannel *aChannel)
 }
 
 nsresult
-nsChannelClassifier::StartInternal(nsIChannel *aChannel)
+nsChannelClassifier::StartInternal()
 {
     // Should only be called in the parent process.
     MOZ_ASSERT(XRE_GetProcessType() == GeckoProcessType_Default);
@@ -231,18 +235,18 @@ nsChannelClassifier::StartInternal(nsIChannel *aChannel)
     // Don't bother to run the classifier on a load that has already failed.
     // (this might happen after a redirect)
     nsresult status;
-    aChannel->GetStatus(&status);
+    mChannel->GetStatus(&status);
     if (NS_FAILED(status))
         return status;
 
     // Don't bother to run the classifier on a cached load that was
     // previously classified as good.
-    if (HasBeenClassified(aChannel)) {
+    if (HasBeenClassified(mChannel)) {
         return NS_ERROR_UNEXPECTED;
     }
 
     nsCOMPtr<nsIURI> uri;
-    nsresult rv = aChannel->GetURI(getter_AddRefs(uri));
+    nsresult rv = mChannel->GetURI(getter_AddRefs(uri));
     NS_ENSURE_SUCCESS(rv, rv);
 
     // Don't bother checking certain types of URIs.
@@ -285,13 +289,13 @@ nsChannelClassifier::StartInternal(nsIChannel *aChannel)
     NS_ENSURE_SUCCESS(rv, rv);
 
     nsCOMPtr<nsIPrincipal> principal;
-    rv = securityManager->GetChannelResultPrincipal(aChannel,
+    rv = securityManager->GetChannelResultPrincipal(mChannel,
                                                     getter_AddRefs(principal));
     NS_ENSURE_SUCCESS(rv, rv);
 
     bool expectCallback;
     bool trackingProtectionEnabled = false;
-    (void)ShouldEnableTrackingProtection(aChannel, &trackingProtectionEnabled);
+    (void)ShouldEnableTrackingProtection(mChannel, &trackingProtectionEnabled);
 
     rv = uriClassifier->Classify(principal, trackingProtectionEnabled, this,
                                  &expectCallback);
@@ -302,7 +306,7 @@ nsChannelClassifier::StartInternal(nsIChannel *aChannel)
     if (expectCallback) {
         // Suspend the channel, it will be resumed when we get the classifier
         // callback.
-        rv = aChannel->Suspend();
+        rv = mChannel->Suspend();
         if (NS_FAILED(rv)) {
             // Some channels (including nsJSChannel) fail on Suspend.  This
             // shouldn't be fatal, but will prevent malware from being
@@ -460,7 +464,7 @@ nsChannelClassifier::OnClassifyComplete(nsresult aErrorCode)
     // Should only be called in the parent process.
     MOZ_ASSERT(XRE_GetProcessType() == GeckoProcessType_Default);
 
-    LOG(("nsChannelClassifier[%p]:OnClassifyComplete", this));
+    LOG(("nsChannelClassifier[%p]:OnClassifyComplete %d", this, aErrorCode));
     if (mSuspendedChannel) {
         MarkEntryClassified(aErrorCode);
 
@@ -488,14 +492,14 @@ nsChannelClassifier::OnClassifyComplete(nsresult aErrorCode)
              "OnClassifyComplete", this, mChannel.get()));
         mChannel->Resume();
     }
-    nsresult rv;
-    nsCOMPtr<nsIHttpChannelInternal> channel = do_QueryInterface(mChannel, &rv);
-    // Even if we have cancelled the channel, we need to call
+
+    // Even if we have cancelled the channel, we may need to call
     // ContinueBeginConnect so that we abort appropriately.
-    if (NS_SUCCEEDED(rv)) {
-        channel->ContinueBeginConnect();
+    if (mChannelInternal) {
+        mChannelInternal->ContinueBeginConnect();
     }
     mChannel = nullptr;
+    mChannelInternal = nullptr;
 
     return NS_OK;
 }

netwerk/base/nsChannelClassifier.h

@@ -10,7 +10,7 @@
 #include "mozilla/Attributes.h"
 
 class nsIChannel;
-
+class nsIHttpChannelInternal;
 
 class nsChannelClassifier MOZ_FINAL : public nsIURIClassifierCallback
 {
@@ -21,10 +21,10 @@ public:
     NS_DECL_NSIURICLASSIFIERCALLBACK
 
     // Calls nsIURIClassifier.Classify with the principal of the given channel,
-    // and cancels the channel on a bad verdict.  If aChannel is
-    // nsIHttpChannelInternal, nsChannelClassifier must call
-    // ContinueBeginConnect once Start has successfully returned.
-    void Start(nsIChannel *aChannel);
+    // and cancels the channel on a bad verdict.  If callContinueBeginConnect is true,
+    // and aChannel is an nsIHttpChannelInternal, nsChannelClassifier must call
+    // nsIHttpChannelInternal.ContinueBeginConnect once Start has returned.
+    void Start(nsIChannel *aChannel, bool aContinueBeginConnect);
     // Whether or not tracking protection should be enabled on this channel.
     nsresult ShouldEnableTrackingProtection(nsIChannel *aChannel, bool *result);
 
@@ -34,6 +34,7 @@ private:
     // True if the channel has been suspended.
     bool mSuspendedChannel;
     nsCOMPtr<nsIChannel> mChannel;
+    nsCOMPtr<nsIHttpChannelInternal> mChannelInternal;
 
     ~nsChannelClassifier() {}
     // Caches good classifications for the channel principal.
@@ -42,7 +43,7 @@ private:
     // Helper function so that we ensure we call ContinueBeginConnect once
     // Start is called. Returns NS_OK if and only if we will get a callback
     // from the classifier service.
-    nsresult StartInternal(nsIChannel *aChannel);
+    nsresult StartInternal();
 
 public:
     // If we are blocking tracking content, update the corresponding flag in

netwerk/protocol/http/nsHttpChannel.cpp

@@ -4902,11 +4902,15 @@ nsHttpChannel::BeginConnect()
             nsCOMPtr<nsIPrincipal> principal = GetPrincipal(false);
             bool tp = false;
             channelClassifier->ShouldEnableTrackingProtection(this, &tp);
-            nsresult response = NS_OK;
-            classifier->ClassifyLocal(principal, tp, &response);
-            if (NS_FAILED(response)) {
-                LOG(("nsHttpChannel::Found principal on local blocklist [this=%p]", this));
-                mLocalBlocklist = true;
+            // See bug 1122691
+            if (tp) {
+                nsresult response = NS_OK;
+                classifier->ClassifyLocal(principal, tp, &response);
+                if (NS_FAILED(response)) {
+                    LOG(("nsHttpChannel::Found principal on local blocklist "
+                         "[this=%p]", this));
+                    mLocalBlocklist = true;
+                }
             }
         }
     }
@@ -4973,16 +4977,26 @@ nsHttpChannel::BeginConnect()
         }
         mCaps &= ~NS_HTTP_ALLOW_PIPELINING;
     }
+    // mLocalBlocklist is true only if the URI is not a tracking domain, it
+    // makes not guarantees about phishing or malware, so we must call
+    // nsChannelClassifier to catch phishing and malware URIs.
+    bool callContinueBeginConnect = true;
     if (mCanceled || !mLocalBlocklist) {
-        return ContinueBeginConnect();
+        rv = ContinueBeginConnect();
+        if (NS_FAILED(rv)) {
+            return rv;
+        }
+        callContinueBeginConnect = false;
+    }
+    // nsChannelClassifier calls ContinueBeginConnect if it has not already
+    // been called, after optionally cancelling the channel once we have a
+    // remote verdict. We call a concrete class instead of an nsI* that might
+    // be overridden.
+    if (!mCanceled) {
+      LOG(("nsHttpChannel::Starting nsChannelClassifier %p [this=%p]",
+           channelClassifier.get(), this));
+      channelClassifier->Start(this, callContinueBeginConnect);
     }
-    MOZ_ASSERT(!mCanceled && mLocalBlocklist);
-    // nsChannelClassifier must call ContinueBeginConnect after optionally
-    // cancelling the channel once we have a remote verdict. We call a concrete
-    // class instead of an nsI* that might be overridden.
-    LOG(("nsHttpChannel::Starting nsChannelClassifier %p [this=%p]",
-         channelClassifier.get(), this));
-    channelClassifier->Start(this);
     return NS_OK;
 }
 

toolkit/components/url-classifier/nsUrlClassifierDBService.cpp

@@ -1132,6 +1132,9 @@ nsUrlClassifierDBService::Init()
   if (NS_FAILED(rv)) {
     rv = NS_GetSpecialDirectory(NS_APP_USER_PROFILE_50_DIR,
                                 getter_AddRefs(cacheDir));
+    if (NS_FAILED(rv)) {
+      return rv;
+    }
   }
 
   // Start the background thread.