Revert of [subresource_filter] Remove Forwarding NavigationThrottles

Revert of [subresource_filter] Remove Forwarding NavigationThrottles (patchset #4 id:60001 of https://codereview.chromium.org/2889913002/ )

Reason for revert:
Seems to introduce an access into an empty vector on ChromiumOS dbg, making lots of tests crash. See
https://uberchromegw.corp.google.com/i/chromium.chromiumos/builders/Linux%20ChromiumOS%20Tests%20%28dbg%29%281%29

Original issue's description:
> [subresource_filter] Remove Forwarding NavigationThrottles
> 
> Now that all activation goes through the safe browsing throttle, we can
> just hook up everything there without doing any sort of persistence in the driver
> factory.
> 
> BUG=708181
> 
> Review-Url: https://codereview.chromium.org/2889913002
> Cr-Commit-Position: refs/heads/master@{#473082}
> Committed: https://chromium.googlesource.com/chromium/src/+/991638fa4c75139e0ce663e27c56299e9849faae

TBR=shivanisha@chromium.org,creis@chromium.org,csharrison@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=708181
Review-Url: https://codereview.chromium.org/2897613002
Cr-Commit-Position: refs/heads/master@{#473156}
Committed: https://chromium.googlesource.com/chromium/src/+/57128089ddde22a0c0bec8d5dab52d3eaee441b0

[Marc Treib, 2017-05-19 10:11:33]

The CQ bit was checked by treib@chromium.org

[Marc Treib, 2017-05-19 10:11:33]

Created Revert of [subresource_filter] Remove Forwarding NavigationThrottles

[commit-bot: I haz the power, 2017-05-19 10:11:55]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-19 10:12:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-19 10:12:15]

Failed to apply patch for
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:
While running git apply --index -3 -p1;
  error: patch failed:
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:75
  Falling back to three-way merge...
  Applied patch to
'components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc'
with conflicts.
  U
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc

Patch:      
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc
Index:
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc
diff --git
a/components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc
b/components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc
index
18112bd3937a69c650af2344ced5fc1346be57ce..15368f6e513fc29968a5f68dacc0806648ee60e5
100644
---
a/components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc
+++
b/components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc
@@ -17,6 +17,7 @@
 #include "components/subresource_filter/core/common/activation_list.h"
 #include "components/subresource_filter/core/common/activation_state.h"
 #include "content/public/browser/navigation_handle.h"
+#include "content/public/browser/navigation_throttle.h"
 #include "content/public/browser/web_contents.h"
 #include "net/base/net_errors.h"
 #include "url/gurl.h"
@@ -27,6 +28,10 @@
 namespace subresource_filter {
 
 namespace {
+
+std::string DistillURLToHostAndPath(const GURL& url) {
+  return url.host() + url.path();
+}
 
 // Returns true with a probability given by |performance_measurement_rate| if
 // ThreadTicks is supported, otherwise returns false.
@@ -75,62 +80,18 @@
 ContentSubresourceFilterDriverFactory::
     ~ContentSubresourceFilterDriverFactory() {}
 
-void ContentSubresourceFilterDriverFactory::OnSafeBrowsingMatchComputed(
-    content::NavigationHandle* navigation_handle,
-    safe_browsing::SBThreatType threat_type,
-    safe_browsing::ThreatPatternType threat_type_metadata) {
-  DCHECK(navigation_handle->IsInMainFrame());
-  DCHECK(!navigation_handle->IsSameDocument());
-  if (navigation_handle->GetNetErrorCode() != net::OK)
-    return;
-
-  ActivationList activation_list =
-      GetListForThreatTypeAndMetadata(threat_type, threat_type_metadata);
-  const GURL& url = navigation_handle->GetURL();
-  const content::Referrer& referrer = navigation_handle->GetReferrer();
-  ui::PageTransition transition = navigation_handle->GetPageTransition();
-
-  if (activation_options_.should_whitelist_site_on_reload &&
-      NavigationIsPageReload(url, referrer, transition)) {
-    // Whitelist this host for the current as well as subsequent navigations.
-    client_->WhitelistInCurrentWebContents(url);
-  }
-
-  ComputeActivationForMainFrameNavigation(navigation_handle, activation_list);
-  DCHECK_NE(activation_decision_, ActivationDecision::UNKNOWN);
-
-  // Check for whitelisted status last, so that the client gets an accurate
-  // indication of whether there would be activation otherwise.
-  bool whitelisted = client_->OnPageActivationComputed(
-      navigation_handle,
-      activation_options_.activation_level == ActivationLevel::ENABLED);
-
-  // Only reset the activation decision reason if we would have activated.
-  if (whitelisted && activation_decision_ == ActivationDecision::ACTIVATED) {
-    TRACE_EVENT0(TRACE_DISABLED_BY_DEFAULT("loading"),
"ActivationWhitelisted");
-    activation_decision_ = ActivationDecision::URL_WHITELISTED;
-    activation_options_ = Configuration::ActivationOptions();
-  }
-
-  if (activation_decision_ != ActivationDecision::ACTIVATED) {
-    DCHECK_EQ(activation_options_.activation_level, ActivationLevel::DISABLED);
-    return;
-  }
-
-  DCHECK_NE(activation_options_.activation_level, ActivationLevel::DISABLED);
-  ActivationState state =
ActivationState(activation_options_.activation_level);
-  state.measure_performance = ShouldMeasurePerformanceForPageLoad(
-      activation_options_.performance_measurement_rate);
-  // TODO(csharrison): Set state.enable_logging based on metadata returns from
-  // the safe browsing filter, when it is available. Add tests for this
-  // behavior.
-  throttle_manager_->NotifyPageActivationComputed(navigation_handle, state);
+void ContentSubresourceFilterDriverFactory::
+    OnMainResourceMatchedSafeBrowsingBlacklist(
+        const GURL& url,
+        safe_browsing::SBThreatType threat_type,
+        safe_browsing::ThreatPatternType threat_type_metadata) {
+  AddActivationListMatch(
+      url, GetListForThreatTypeAndMetadata(threat_type, threat_type_metadata));
 }
 
 void ContentSubresourceFilterDriverFactory::
     ComputeActivationForMainFrameNavigation(
-        content::NavigationHandle* navigation_handle,
-        ActivationList matched_list) {
+        content::NavigationHandle* navigation_handle) {
   const GURL& url(navigation_handle->GetURL());
 
   if (!url.SchemeIsHTTPOrHTTPS()) {
@@ -143,9 +104,9 @@
   const auto highest_priority_activated_config =
       std::find_if(config_list->configs_by_decreasing_priority().begin(),
                    config_list->configs_by_decreasing_priority().end(),
-                   [&url, matched_list, this](const Configuration& config) {
+                   [&url, this](const Configuration& config) {
                      return DoesMainFrameURLSatisfyActivationConditions(
-                         url, config.activation_conditions, matched_list);
+                         url, config.activation_conditions);
                    });
 
   bool has_activated_config =
@@ -174,16 +135,16 @@
 bool ContentSubresourceFilterDriverFactory::
     DoesMainFrameURLSatisfyActivationConditions(
         const GURL& url,
-        const Configuration::ActivationConditions& conditions,
-        ActivationList matched_list) const {
+        const Configuration::ActivationConditions& conditions) const {
   switch (conditions.activation_scope) {
     case ActivationScope::ALL_SITES:
       return true;
     case ActivationScope::ACTIVATION_LIST:
-      if (conditions.activation_list == matched_list)
+      if (DidURLMatchActivationList(url, conditions.activation_list))
         return true;
       if (conditions.activation_list == ActivationList::PHISHING_INTERSTITIAL
&&
-          matched_list == ActivationList::SOCIAL_ENG_ADS_INTERSTITIAL) {
+          DidURLMatchActivationList(
+              url, ActivationList::SOCIAL_ENG_ADS_INTERSTITIAL)) {
         // Handling special case, where activation on the phishing sites also
         // mean the activation on the sites with social engineering metadata.
         return true;
@@ -212,6 +173,55 @@
   web_contents()->GetController().Reload(content::ReloadType::NORMAL, true);
 }
 
+void ContentSubresourceFilterDriverFactory::WillProcessResponse(
+    content::NavigationHandle* navigation_handle) {
+  DCHECK(!navigation_handle->IsSameDocument());
+  if (!navigation_handle->IsInMainFrame() ||
+      navigation_handle->GetNetErrorCode() != net::OK) {
+    return;
+  }
+
+  const GURL& url = navigation_handle->GetURL();
+  const content::Referrer& referrer = navigation_handle->GetReferrer();
+  ui::PageTransition transition = navigation_handle->GetPageTransition();
+
+  if (activation_options_.should_whitelist_site_on_reload &&
+      NavigationIsPageReload(url, referrer, transition)) {
+    // Whitelist this host for the current as well as subsequent navigations.
+    client_->WhitelistInCurrentWebContents(url);
+  }
+
+  ComputeActivationForMainFrameNavigation(navigation_handle);
+  DCHECK_NE(activation_decision_, ActivationDecision::UNKNOWN);
+
+  // Check for whitelisted status last, so that the client gets an accurate
+  // indication of whether there would be activation otherwise.
+  bool whitelisted = client_->OnPageActivationComputed(
+      navigation_handle,
+      activation_options_.activation_level == ActivationLevel::ENABLED);
+
+  // Only reset the activation decision reason if we would have activated.
+  if (whitelisted && activation_decision_ == ActivationDecision::ACTIVATED) {
+    TRACE_EVENT0(TRACE_DISABLED_BY_DEFAULT("loading"),
"ActivationWhitelisted");
+    activation_decision_ = ActivationDecision::URL_WHITELISTED;
+    activation_options_ = Configuration::ActivationOptions();
+  }
+
+  if (activation_decision_ != ActivationDecision::ACTIVATED) {
+    DCHECK_EQ(activation_options_.activation_level, ActivationLevel::DISABLED);
+    return;
+  }
+
+  DCHECK_NE(activation_options_.activation_level, ActivationLevel::DISABLED);
+  ActivationState state =
ActivationState(activation_options_.activation_level);
+  state.measure_performance = ShouldMeasurePerformanceForPageLoad(
+      activation_options_.performance_measurement_rate);
+  // TODO(csharrison): Set state.enable_logging based on metadata returns from
+  // the safe browsing filter, when it is available. Add tests for this
+  // behavior.
+  throttle_manager_->NotifyPageActivationComputed(navigation_handle, state);
+}
+
 void ContentSubresourceFilterDriverFactory::OnFirstSubresourceLoadDisallowed()
{
   if (activation_options_.should_suppress_notifications)
     return;
@@ -224,6 +234,7 @@
   if (navigation_handle->IsInMainFrame() &&
       !navigation_handle->IsSameDocument()) {
     activation_decision_ = ActivationDecision::UNKNOWN;
+    activation_list_matches_.clear();
     client_->ToggleNotificationVisibility(false);
   }
 }
@@ -239,4 +250,22 @@
   }
 }
 
+bool ContentSubresourceFilterDriverFactory::DidURLMatchActivationList(
+    const GURL& url,
+    ActivationList activation_list) const {
+  auto match_types =
+      activation_list_matches_.find(DistillURLToHostAndPath(url));
+  return match_types != activation_list_matches_.end() &&
+  …
(message too large)

[Marc Treib, 2017-05-19 10:58:31]

The CQ bit was checked by treib@chromium.org

[Charlie Harrison, 2017-05-19 10:58:41]

I will apply the revert manually, sorry about this!

[Charlie Harrison, 2017-05-19 10:58:42]

I will apply the revert manually, sorry about this!

[commit-bot: I haz the power, 2017-05-19 10:58:58]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-19 10:59:33]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1495191511033860, "parent_rev":
"6c4e650dc5f0b6f20a9669c0943bd20c18e85ed1", "commit_rev":
"57128089ddde22a0c0bec8d5dab52d3eaee441b0"}

[Charlie Harrison, 2017-05-19 10:59:50]

On 2017/05/19 10:58:58, commit-bot: I haz the power wrote:
> CQ is trying da patch.
> 
> Follow status at:
>
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

Oh nevermind, I saw that you combined the two dependent patches.

[commit-bot: I haz the power, 2017-05-19 10:59:52]

Description was changed from

==========
Revert of [subresource_filter] Remove Forwarding NavigationThrottles (patchset
#4 id:60001 of https://codereview.chromium.org/2889913002/ )

Reason for revert:
Seems to introduce an access into an empty vector on ChromiumOS dbg, making lots
of tests crash. See
https://uberchromegw.corp.google.com/i/chromium.chromiumos/builders/Linux%20C...

Original issue's description:
> [subresource_filter] Remove Forwarding NavigationThrottles
> 
> Now that all activation goes through the safe browsing throttle, we can
> just hook up everything there without doing any sort of persistence in the
driver
> factory.
> 
> BUG=708181
> 
> Review-Url: https://codereview.chromium.org/2889913002
> Cr-Commit-Position: refs/heads/master@{#473082}
> Committed:
https://chromium.googlesource.com/chromium/src/+/991638fa4c75139e0ce663e27c56...

TBR=shivanisha@chromium.org,creis@chromium.org,csharrison@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=708181
==========

to

==========
Revert of [subresource_filter] Remove Forwarding NavigationThrottles (patchset
#4 id:60001 of https://codereview.chromium.org/2889913002/ )

Reason for revert:
Seems to introduce an access into an empty vector on ChromiumOS dbg, making lots
of tests crash. See
https://uberchromegw.corp.google.com/i/chromium.chromiumos/builders/Linux%20C...

Original issue's description:
> [subresource_filter] Remove Forwarding NavigationThrottles
> 
> Now that all activation goes through the safe browsing throttle, we can
> just hook up everything there without doing any sort of persistence in the
driver
> factory.
> 
> BUG=708181
> 
> Review-Url: https://codereview.chromium.org/2889913002
> Cr-Commit-Position: refs/heads/master@{#473082}
> Committed:
https://chromium.googlesource.com/chromium/src/+/991638fa4c75139e0ce663e27c56...

TBR=shivanisha@chromium.org,creis@chromium.org,csharrison@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=708181

Review-Url: https://codereview.chromium.org/2897613002
Cr-Commit-Position: refs/heads/master@{#473156}
Committed:
https://chromium.googlesource.com/chromium/src/+/57128089ddde22a0c0bec8d5dab5...
==========

[commit-bot: I haz the power, 2017-05-19 10:59:53]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/57128089ddde22a0c0bec8d5dab5...

[Marc Treib, 2017-05-19 11:00:27]

On 2017/05/19 10:58:42, Charlie Harrison wrote:
> I will apply the revert manually, sorry about this!

I just reverted the later CL that depended on this. Now the revert should
hopefully land cleanly.
(Sorry if I've made your life harder than necessary by this...)

[Charlie Harrison, 2017-05-19 11:02:11]

On 2017/05/19 11:00:27, Marc Treib wrote:
> On 2017/05/19 10:58:42, Charlie Harrison wrote:
> > I will apply the revert manually, sorry about this!
> 
> I just reverted the later CL that depended on this. Now the revert should
> hopefully land cleanly.
> (Sorry if I've made your life harder than necessary by this...)

Please, don't worry about it! Quick reverts make everyone's life less painful.