Bug 1130893 - Use GetChannelURIPrincipal instead of GetChannelResultPrincipal in nsChannelClassifier, only call nsChannelClassifier if LOAD_CLASSIFY_URI is set. r=mcmanus r=ckerschb

netwerk/base/nsChannelClassifier.cpp

@@ -150,6 +150,16 @@ nsChannelClassifier::ShouldEnableTrackingProtection(nsIChannel *aChannel,
     // the security state. If any channels are subsequently cancelled
     // (page elements blocked) the state will be then updated.
     if (*result) {
+#ifdef DEBUG
+      nsCString topspec;
+      nsCString spec;
+      uri->GetSpec(topspec);
+      aChannel->GetURI(getter_AddRefs(uri));
+      uri->GetSpec(spec);
+      LOG(("nsChannelClassifier[%p]: Enabling tracking protection checks on channel[%p] "
+           "with uri %s for toplevel window %s", this, aChannel, spec.get(),
+           topspec.get()));
+#endif
       return NS_OK;
     }
 
@@ -289,14 +299,25 @@ nsChannelClassifier::StartInternal()
     NS_ENSURE_SUCCESS(rv, rv);
 
     nsCOMPtr<nsIPrincipal> principal;
-    rv = securityManager->GetChannelResultPrincipal(mChannel,
+    rv = securityManager->GetChannelURIPrincipal(mChannel, getter_AddRefs(principal));
-                                                    getter_AddRefs(principal));
     NS_ENSURE_SUCCESS(rv, rv);
 
     bool expectCallback;
     bool trackingProtectionEnabled = false;
     (void)ShouldEnableTrackingProtection(mChannel, &trackingProtectionEnabled);
 
+#ifdef DEBUG
+    {
+      nsCString uriSpec;
+      uri->GetSpec(uriSpec);
+      nsCOMPtr<nsIURI> principalURI;
+      principal->GetURI(getter_AddRefs(principalURI));
+      nsCString principalSpec;
+      principalURI->GetSpec(principalSpec);
+      LOG(("nsChannelClassifier: Classifying principal %s on channel with uri %s "
+           "[this=%p]", principalSpec.get(), uriSpec.get(), this));
+    }
+#endif
     rv = uriClassifier->Classify(principal, trackingProtectionEnabled, this,
                                  &expectCallback);
     if (NS_FAILED(rv)) {

netwerk/protocol/http/nsHttpChannel.cpp

@@ -4853,14 +4853,20 @@ nsHttpChannel::BeginConnect()
             nsCOMPtr<nsIPrincipal> principal = GetPrincipal(false);
             bool tp = false;
             channelClassifier->ShouldEnableTrackingProtection(this, &tp);
-            // See bug 1122691
+            // We skip speculative connections by setting mLocalBlocklist only
+            // when tracking protection is enabled. Though we could do this for
+            // both phishing and malware, it is not necessary for correctness,
+            // since no network events will be received while the
+            // nsChannelClassifier is in progress. 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 "
+                    LOG(("nsHttpChannel::ClassifyLocal found principal on local "
-                         "[this=%p]", this));
+                         "blocklist [this=%p]", this));
                     mLocalBlocklist = true;
+                } else {
+                    LOG(("nsHttpChannel::ClassifyLocal no result found [this=%p]", this));
                 }
             }
         }
@@ -4928,25 +4934,29 @@ nsHttpChannel::BeginConnect()
         }
         mCaps &= ~NS_HTTP_ALLOW_PIPELINING;
     }
-    // mLocalBlocklist is true only if the URI is not a tracking domain, it
+    if (!(mLoadFlags & LOAD_CLASSIFY_URI)) {
-    // makes not guarantees about phishing or malware, so we must call
+        return ContinueBeginConnect();
+    }
+    // mLocalBlocklist is true only if tracking protection is enabled and the
+    // URI is a tracking domain, it makes no guarantees about phishing or
+    // malware, so if LOAD_CLASSIFY_URI is true we must call
     // nsChannelClassifier to catch phishing and malware URIs.
     bool callContinueBeginConnect = true;
     if (mCanceled || !mLocalBlocklist) {
-        rv = ContinueBeginConnect();
+       rv = ContinueBeginConnect();
-        if (NS_FAILED(rv)) {
+       if (NS_FAILED(rv)) {
-            return rv;
+           return rv;
-        }
+       }
-        callContinueBeginConnect = false;
+       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]",
+        LOG(("nsHttpChannel::Starting nsChannelClassifier %p [this=%p]",
            channelClassifier.get(), this));
-      channelClassifier->Start(this, callContinueBeginConnect);
+        channelClassifier->Start(this, callContinueBeginConnect);
     }
     return NS_OK;
 }