Bug 1100024: CLOSED TREE Backout https://hg.mozilla.org/integration/mozilla-inbound/rev/671ad56e6e12 for causing crashes on ClassifyLocal lookups (r=backout)

netwerk/base/src/nsBaseChannel.cpp

@@ -289,10 +289,15 @@ nsBaseChannel::ClassifyURI()
     return;
   }
 
+  nsresult rv;
+
   if (mLoadFlags & LOAD_CLASSIFY_URI) {
     nsRefPtr<nsChannelClassifier> classifier = new nsChannelClassifier();
     if (classifier) {
-      classifier->Start(this);
+      rv = classifier->Start(this);
+      if (NS_FAILED(rv)) {
+        Cancel(rv);
+      }
     } else {
       Cancel(NS_ERROR_OUT_OF_MEMORY);
     }

netwerk/base/src/nsChannelClassifier.cpp

@@ -49,8 +49,7 @@ NS_IMPL_ISUPPORTS(nsChannelClassifier,
                   nsIURIClassifierCallback)
 
 nsChannelClassifier::nsChannelClassifier()
-  : mIsAllowListed(false),
-    mSuspendedChannel(false)
+  : mIsAllowListed(false)
 {
 #if defined(PR_LOGGING)
     if (!gChannelClassifierLog)
@@ -210,20 +209,8 @@ nsChannelClassifier::NotifyTrackingProtectionDisabled(nsIChannel *aChannel)
     return NS_OK;
 }
 
-void
-nsChannelClassifier::Start(nsIChannel *aChannel)
-{
-  mChannel = aChannel;
-  nsresult rv = StartInternal(aChannel);
-  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.
-    OnClassifyComplete(NS_OK);
-  }
-}
-
 nsresult
-nsChannelClassifier::StartInternal(nsIChannel *aChannel)
+nsChannelClassifier::Start(nsIChannel *aChannel)
 {
     // Should only be called in the parent process.
     MOZ_ASSERT(XRE_GetProcessType() == GeckoProcessType_Default);
@@ -233,12 +220,12 @@ nsChannelClassifier::StartInternal(nsIChannel *aChannel)
     nsresult status;
     aChannel->GetStatus(&status);
     if (NS_FAILED(status))
-        return status;
+        return NS_OK;
 
     // Don't bother to run the classifier on a cached load that was
-    // previously classified as good.
+    // previously classified.
     if (HasBeenClassified(aChannel)) {
-        return NS_ERROR_UNEXPECTED;
+        return NS_OK;
     }
 
     nsCOMPtr<nsIURI> uri;
@@ -251,32 +238,32 @@ nsChannelClassifier::StartInternal(nsIChannel *aChannel)
                              nsIProtocolHandler::URI_DANGEROUS_TO_LOAD,
                              &hasFlags);
     NS_ENSURE_SUCCESS(rv, rv);
-    if (hasFlags) return NS_ERROR_UNEXPECTED;
+    if (hasFlags) return NS_OK;
 
     rv = NS_URIChainHasFlags(uri,
                              nsIProtocolHandler::URI_IS_LOCAL_FILE,
                              &hasFlags);
     NS_ENSURE_SUCCESS(rv, rv);
-    if (hasFlags) return NS_ERROR_UNEXPECTED;
+    if (hasFlags) return NS_OK;
 
     rv = NS_URIChainHasFlags(uri,
                              nsIProtocolHandler::URI_IS_UI_RESOURCE,
                              &hasFlags);
     NS_ENSURE_SUCCESS(rv, rv);
-    if (hasFlags) return NS_ERROR_UNEXPECTED;
+    if (hasFlags) return NS_OK;
 
     rv = NS_URIChainHasFlags(uri,
                              nsIProtocolHandler::URI_IS_LOCAL_RESOURCE,
                              &hasFlags);
     NS_ENSURE_SUCCESS(rv, rv);
-    if (hasFlags) return NS_ERROR_UNEXPECTED;
+    if (hasFlags) return NS_OK;
 
     nsCOMPtr<nsIURIClassifier> uriClassifier =
         do_GetService(NS_URICLASSIFIERSERVICE_CONTRACTID, &rv);
     if (rv == NS_ERROR_FACTORY_NOT_REGISTERED ||
         rv == NS_ERROR_NOT_AVAILABLE) {
         // no URI classifier, ignore this failure.
-        return NS_ERROR_NOT_AVAILABLE;
+        return NS_OK;
     }
     NS_ENSURE_SUCCESS(rv, rv);
 
@@ -295,9 +282,7 @@ nsChannelClassifier::StartInternal(nsIChannel *aChannel)
 
     rv = uriClassifier->Classify(principal, trackingProtectionEnabled, this,
                                  &expectCallback);
-    if (NS_FAILED(rv)) {
-        return rv;
-    }
+    if (NS_FAILED(rv)) return rv;
 
     if (expectCallback) {
         // Suspend the channel, it will be resumed when we get the classifier
@@ -307,16 +292,14 @@ nsChannelClassifier::StartInternal(nsIChannel *aChannel)
             // Some channels (including nsJSChannel) fail on Suspend.  This
             // shouldn't be fatal, but will prevent malware from being
             // blocked on these channels.
-            LOG(("nsChannelClassifier[%p]: Couldn't suspend channel", this));
-            return rv;
+            return NS_OK;
         }
 
-        mSuspendedChannel = true;
+        mSuspendedChannel = aChannel;
+#ifdef DEBUG
         LOG(("nsChannelClassifier[%p]: suspended channel %p",
-             this, mChannel.get()));
-    } else {
-        LOG(("nsChannelClassifier[%p]: not expecting callback", this));
-        return NS_ERROR_FAILURE;
+             this, mSuspendedChannel.get()));
+#endif
     }
 
     return NS_OK;
@@ -335,7 +318,8 @@ nsChannelClassifier::MarkEntryClassified(nsresult status)
         return;
     }
 
-    nsCOMPtr<nsICachingChannel> cachingChannel = do_QueryInterface(mChannel);
+    nsCOMPtr<nsICachingChannel> cachingChannel =
+        do_QueryInterface(mSuspendedChannel);
     if (!cachingChannel) {
         return;
     }
@@ -460,18 +444,17 @@ nsChannelClassifier::OnClassifyComplete(nsresult aErrorCode)
     // Should only be called in the parent process.
     MOZ_ASSERT(XRE_GetProcessType() == GeckoProcessType_Default);
 
-    LOG(("nsChannelClassifier[%p]:OnClassifyComplete", this));
     if (mSuspendedChannel) {
         MarkEntryClassified(aErrorCode);
 
         if (NS_FAILED(aErrorCode)) {
 #ifdef DEBUG
             nsCOMPtr<nsIURI> uri;
-            mChannel->GetURI(getter_AddRefs(uri));
+            mSuspendedChannel->GetURI(getter_AddRefs(uri));
             nsCString spec;
             uri->GetSpec(spec);
             LOG(("nsChannelClassifier[%p]: cancelling channel %p for %s "
-                 "with error code: %x", this, mChannel.get(),
+                 "with error code: %x", this, mSuspendedChannel.get(),
                  spec.get(), aErrorCode));
 #endif
 
@@ -479,23 +462,18 @@ nsChannelClassifier::OnClassifyComplete(nsresult aErrorCode)
             // Do update the security state of the document and fire a security
             // change event.
             if (aErrorCode == NS_ERROR_TRACKING_URI) {
-              SetBlockedTrackingContent(mChannel);
+              SetBlockedTrackingContent(mSuspendedChannel);
             }
 
-            mChannel->Cancel(aErrorCode);
-        }
+            mSuspendedChannel->Cancel(aErrorCode);
+          }
+#ifdef DEBUG
         LOG(("nsChannelClassifier[%p]: resuming channel %p from "
-             "OnClassifyComplete", this, mChannel.get()));
-        mChannel->Resume();
+             "OnClassifyComplete", this, mSuspendedChannel.get()));
+#endif
+        mSuspendedChannel->Resume();
+        mSuspendedChannel = nullptr;
     }
-    nsresult rv;
-    nsCOMPtr<nsIHttpChannelInternal> channel = do_QueryInterface(mChannel, &rv);
-    // Even if we have cancelled the channel, we need to call
-    // ContinueBeginConnect so that we abort appropriately.
-    if (NS_SUCCEEDED(rv)) {
-        channel->ContinueBeginConnect();
-    }
-    mChannel = nullptr;
 
     return NS_OK;
 }

netwerk/base/src/nsChannelClassifier.h

@@ -20,27 +20,16 @@ public:
     NS_DECL_ISUPPORTS
     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);
+    nsresult Start(nsIChannel *aChannel);
 
 private:
-    // True if the channel is on the allow list.
+    nsCOMPtr<nsIChannel> mSuspendedChannel;
+    // Set true if the channel is on the allow list.
     bool mIsAllowListed;
-    // True if the channel has been suspended.
-    bool mSuspendedChannel;
-    nsCOMPtr<nsIChannel> mChannel;
 
     ~nsChannelClassifier() {}
-    // Caches good classifications for the channel principal.
     void MarkEntryClassified(nsresult status);
     bool HasBeenClassified(nsIChannel *aChannel);
-    // 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);
     // Whether or not tracking protection should be enabled on this channel.
     nsresult ShouldEnableTrackingProtection(nsIChannel *aChannel, bool *result);
 

netwerk/protocol/http/HttpBaseChannel.cpp

@@ -1409,15 +1409,6 @@ HttpBaseChannel::RedirectTo(nsIURI *newURI)
 // HttpBaseChannel::nsIHttpChannelInternal
 //-----------------------------------------------------------------------------
 
-NS_IMETHODIMP
-HttpBaseChannel::ContinueBeginConnect()
-{
-  MOZ_ASSERT(XRE_GetProcessType() != GeckoProcessType_Default,
-             "The parent overrides this");
-  MOZ_ASSERT(false, "This method must be overridden");
-  return NS_ERROR_NOT_IMPLEMENTED;
-}
-
 NS_IMETHODIMP
 HttpBaseChannel::GetTopWindowURI(nsIURI **aTopWindowURI)
 {

netwerk/protocol/http/HttpBaseChannel.h

@@ -188,7 +188,6 @@ public:
   NS_IMETHOD GetLastModifiedTime(PRTime* lastModifiedTime) MOZ_OVERRIDE;
   NS_IMETHOD ForceNoIntercept() MOZ_OVERRIDE;
   NS_IMETHOD GetTopWindowURI(nsIURI **aTopWindowURI) MOZ_OVERRIDE;
-  NS_IMETHOD ContinueBeginConnect();
 
   inline void CleanRedirectCacheChainIfNecessary()
   {

netwerk/protocol/http/nsHttpChannel.cpp

@@ -22,7 +22,6 @@
 #include "nsISeekableStream.h"
 #include "nsILoadGroupChild.h"
 #include "nsIProtocolProxyService2.h"
-#include "nsIURIClassifier.h"
 #include "nsMimeTypes.h"
 #include "nsNetUtil.h"
 #include "prprf.h"
@@ -40,7 +39,6 @@
 #include "GeckoProfiler.h"
 #include "nsIConsoleService.h"
 #include "mozilla/Attributes.h"
-#include "mozilla/Preferences.h"
 #include "mozilla/VisualEventTracer.h"
 #include "nsISSLSocketControl.h"
 #include "sslt.h"
@@ -242,7 +240,6 @@ nsHttpChannel::nsHttpChannel()
     , mIsPartialRequest(0)
     , mHasAutoRedirectVetoNotifier(0)
     , mPushedStream(nullptr)
-    , mLocalBlocklist(false)
     , mDidReval(false)
 {
     LOG(("Creating nsHttpChannel [this=%p]\n", this));
@@ -329,7 +326,7 @@ nsHttpChannel::Connect()
     mConnectionInfo->SetPrivate(mPrivateBrowsing);
     mConnectionInfo->SetNoSpdy(mCaps & NS_HTTP_DISALLOW_SPDY);
 
-    // Consider opening a TCP connection right away.
+    // Consider opening a TCP connection right away
     SpeculativeConnect();
 
     // Don't allow resuming when cache must be used
@@ -436,11 +433,11 @@ nsHttpChannel::SpeculativeConnect()
     // Before we take the latency hit of dealing with the cache, try and
     // get the TCP (and SSL) handshakes going so they can overlap.
 
-    // don't speculate if we are on a local blocklist, on uses of the offline
-    // application cache, if we are offline, when doing http upgrade (i.e.
-    // websockets bootstrap), or if we can't do keep-alive (because then we
-    // couldn't reuse the speculative connection anyhow).
-    if (mLocalBlocklist || mApplicationCache || gIOService->IsOffline() ||
+    // don't speculate on uses of the offline application cache,
+    // if we are offline, when doing http upgrade (i.e. websockets bootstrap),
+    // or if we can't do keep-alive (because then we couldn't reuse
+    // the speculative connection anyhow).
+    if (mApplicationCache || gIOService->IsOffline() ||
         mUpgradeProtocolCallback || !(mCaps & NS_HTTP_ALLOW_KEEPALIVE))
         return;
 
@@ -4876,21 +4873,6 @@ nsHttpChannel::BeginConnect()
     if (mAPIRedirectToURI) {
         return AsyncCall(&nsHttpChannel::HandleAsyncAPIRedirect);
     }
-    // Check to see if this principal exists on local blocklists.
-    if (mLoadFlags & LOAD_CLASSIFY_URI) {
-        nsCOMPtr<nsIURIClassifier> classifier = do_GetService(NS_URICLASSIFIERSERVICE_CONTRACTID);
-        if (classifier) {
-            nsCOMPtr<nsIPrincipal> principal = GetPrincipal(false);
-            bool tp = Preferences::GetBool("privacy.trackingprotection.enabled",
-                                           false);
-            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;
-            }
-        }
-    }
 
     // If mTimingEnabled flag is not set after OnModifyRequest() then
     // clear the already recorded AsyncOpen value for consistency.
@@ -4910,7 +4892,7 @@ nsHttpChannel::BeginConnect()
     if (mLoadFlags & VALIDATE_ALWAYS || BYPASS_LOCAL_CACHE(mLoadFlags))
         mCaps |= NS_HTTP_REFRESH_DNS;
 
-    if (!mLocalBlocklist && !mConnectionInfo->UsingHttpProxy() &&
+    if (!mConnectionInfo->UsingHttpProxy() &&
         !(mLoadFlags & (LOAD_NO_NETWORK_IO | LOAD_ONLY_FROM_CACHE))) {
         // Start a DNS lookup very early in case the real open is queued the DNS can
         // happen in parallel. Do not do so in the presence of an HTTP proxy as
@@ -4954,17 +4936,27 @@ nsHttpChannel::BeginConnect()
         }
         mCaps &= ~NS_HTTP_ALLOW_PIPELINING;
     }
-    if (mCanceled || !mLocalBlocklist) {
-        return ContinueBeginConnect();
+
+    // We may have been cancelled already, either by on-modify-request
+    // listeners or by load group observers; in that case, we should
+    // not send the request to the server
+    if (mCanceled)
+        rv = mStatus;
+    else
+        rv = Connect();
+    if (NS_FAILED(rv)) {
+        LOG(("Calling AsyncAbort [rv=%x mCanceled=%i]\n", rv, mCanceled));
+        CloseCacheEntry(true);
+        AsyncAbort(rv);
+    } else if (mLoadFlags & LOAD_CLASSIFY_URI) {
+        nsRefPtr<nsChannelClassifier> classifier = new nsChannelClassifier();
+        rv = classifier->Start(this);
+        if (NS_FAILED(rv)) {
+            Cancel(rv);
+            return rv;
+        }
     }
-    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.
-    nsRefPtr<nsChannelClassifier> classifier = new nsChannelClassifier();
-    LOG(("nsHttpChannel::Starting nsChannelClassifier %p [this=%p]",
-         classifier.get(), this));
-    classifier->Start(this);
+
     return NS_OK;
 }
 
@@ -5001,30 +4993,6 @@ nsHttpChannel::SetPriority(int32_t value)
     return NS_OK;
 }
 
-//-----------------------------------------------------------------------------
-// nsHttpChannel::nsIHttpChannelInternal
-//-----------------------------------------------------------------------------
-NS_IMETHODIMP
-nsHttpChannel::ContinueBeginConnect()
-{
-    LOG(("nsHttpChannel::ContinueBeginConnect [this=%p]", this));
-    nsresult rv;
-    // We may have been cancelled already, either by on-modify-request
-    // listeners or load group observers or nsChannelClassifier; in that case,
-    // we should not send the request to the server
-    if (mCanceled) {
-        rv = mStatus;
-    } else {
-        rv = Connect();
-    }
-    if (NS_FAILED(rv)) {
-        LOG(("Calling AsyncAbort [rv=%x mCanceled=%i]\n", rv, mCanceled));
-        CloseCacheEntry(true);
-        AsyncAbort(rv);
-    }
-    return rv;
-}
-
 //-----------------------------------------------------------------------------
 // HttpChannel::nsIClassOfService
 //-----------------------------------------------------------------------------

netwerk/protocol/http/nsHttpChannel.h

@@ -119,7 +119,6 @@ public:
     NS_IMETHOD AsyncOpen(nsIStreamListener *listener, nsISupports *aContext) MOZ_OVERRIDE;
     // nsIHttpChannelInternal
     NS_IMETHOD SetupFallbackChannel(const char *aFallbackKey) MOZ_OVERRIDE;
-    NS_IMETHOD ContinueBeginConnect();
     // nsISupportsPriority
     NS_IMETHOD SetPriority(int32_t value) MOZ_OVERRIDE;
     // nsIClassOfService
@@ -466,9 +465,6 @@ private:
     nsRefPtr<nsDNSPrefetch>           mDNSPrefetch;
 
     Http2PushedStream                 *mPushedStream;
-    // True if the channel's principal was found on a phishing, malware, or
-    // tracking (if tracking protection is enabled) blocklist
-    bool                              mLocalBlocklist;
 
     nsresult WaitForRedirectCallback();
     void PushRedirectAsyncFunc(nsContinueRedirectionFunc func);

netwerk/protocol/http/nsIHttpChannelInternal.idl

@@ -224,10 +224,4 @@ interface nsIHttpChannelInternal : nsISupports
      * The URI of the top-level window that's associated with this channel.
      */
     readonly attribute nsIURI topWindowURI;
-
-    /**
-     * Used only by nsChannelClassifier to resume connecting or abort the
-     * channel after a remote classification verdict.
-     */
-    void continueBeginConnect();
 };