Refactoring how conditions without URL attributes are handled.

chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.h"
 
+#include <algorithm>
 #include <limits>
+#include <utility>
 
 #include "chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h"
 #include "chrome/browser/extensions/api/web_request/web_request_api_helpers.h"
 #include "chrome/browser/extensions/api/web_request/web_request_permissions.h"
 #include "chrome/browser/extensions/extension_system.h"
 #include "net/url_request/url_request.h"
 
 namespace extensions {
 
 WebRequestRulesRegistry::WebRequestRulesRegistry(Profile* profile,
                                                  Delegate* delegate)
     : RulesRegistryWithCache(delegate) {
   if (profile)
     extension_info_map_ = ExtensionSystem::Get(profile)->info_map();
 }
 
-std::set<WebRequestRule::GlobalRuleId>
+std::set<const WebRequestRule*>
 WebRequestRulesRegistry::GetMatches(
     const WebRequestRule::RequestData& request_data) {
-  std::set<WebRequestRule::GlobalRuleId> result;
+  typedef std::set<const WebRequestRule*> RuleSet;
+  typedef std::set<URLMatcherConditionSet::ID> URLMatches;
 
-  // Figure out for which rules the URL match conditions were fulfilled.
-  typedef std::set<URLMatcherConditionSet::ID> URLMatches;
+  RuleSet not_fulfilled;
+  RuleSet result;
   URLMatches url_matches = url_matcher_.MatchURL(request_data.request->url());
 
-  // Then we need to check for each of these, whether the other
-  // WebRequestConditionAttributes are also fulfilled.
-  for (URLMatches::iterator url_match = url_matches.begin();
+  // 1st phase -- add all rules with some conditions without UrlFilter
+  // attributes.
+  for (RuleSet::const_iterator it = rules_with_untriggered_conditions_.begin();
+       it != rules_with_untriggered_conditions_.end(); ++it) {
+    if ((*it)->conditions().IsFulfilled(-1, url_matches, request_data))
+      result.insert(*it);
+    else
+      not_fulfilled.insert(*it);
+  }
+
+  // 2nd phase -- add all rules with some conditions triggered by URL matches.
+  for (URLMatches::const_iterator url_match = url_matches.begin();
        url_match != url_matches.end(); ++url_match) {
-    RuleTriggers::iterator rule_trigger = rule_triggers_.find(*url_match);
+    RuleTriggers::const_iterator rule_trigger = rule_triggers_.find(*url_match);
     CHECK(rule_trigger != rule_triggers_.end());
+    if (not_fulfilled.find(rule_trigger->second) == not_fulfilled.end() &&

[Jeffrey Yasskin, 2013/01/10 22:00:57]

Use ContainsKey from base/stl_util.h instead of find?

[Jeffrey Yasskin, 2013/01/10 22:00:57]

Is this right? Say there's a rule with conditions: [ {url: "google.com"},
{contentType: "neverMatch"} ]. Then this rule will end up in
rules_with_untriggered_conditions_, and since it doesn't match with no URL,
it'll end up in not_fulfilled. Now it comes up on a google.com request in this
triggered loop and gets skipped because it wasn't fulfilled with no URL, even
though it would be fulfilled with the current URL.

Could you add a test for this?  (This sort of bug or confusion-on-my-part is why
I pushed for the simplest implementation rather than trying to optimize it by
punching information through the abstraction layers.)

[battre, 2013/01/11 17:41:55]

I think the test we want here is:
if (!ContainsKey(result, rule_trigger->second) &&
rule_trigger->second->conditions().IsFulfilled(...))

If it is fulfilled already, we don't need to test any further.

[vabr (Chromium), 2013/01/15 10:24:57]

Thanks, Jeffrey, for pointing this out, this was a stupid mistake of mine.

I did as Dominic suggested, and also added the unit test.

+        rule_trigger->second->conditions().IsFulfilled(
+            *url_match, url_matches, request_data))
+      result.insert(rule_trigger->second);
+  }
 
-    WebRequestRule* rule = rule_trigger->second;
-    if (rule->conditions().IsFulfilled(*url_match, request_data))
-      result.insert(rule->id());
-  }
   return result;
 }
 
 std::list<LinkedPtrEventResponseDelta> WebRequestRulesRegistry::CreateDeltas(
     const ExtensionInfoMap* extension_info_map,
     const WebRequestRule::RequestData& request_data,
     bool crosses_incognito) {
   if (webrequest_rules_.empty())
     return std::list<LinkedPtrEventResponseDelta>();
 
-  std::set<WebRequestRule::GlobalRuleId> matches =
-      GetMatches(request_data);
+  std::set<const WebRequestRule*> matches = GetMatches(request_data);
 
   // Sort all matching rules by their priority so that they can be processed
   // in decreasing order.
   typedef std::pair<WebRequestRule::Priority, WebRequestRule::GlobalRuleId>
       PriorityRuleIdPair;
   std::vector<PriorityRuleIdPair> ordered_matches;
   ordered_matches.reserve(matches.size());
-  for (std::set<WebRequestRule::GlobalRuleId>::iterator i = matches.begin();
+  for (std::set<const WebRequestRule*>::iterator i = matches.begin();
        i != matches.end(); ++i) {
-    RulesMap::const_iterator rule = webrequest_rules_.find(*i);
-    CHECK(rule != webrequest_rules_.end());
-    ordered_matches.push_back(make_pair(rule->second->priority(), *i));
+    ordered_matches.push_back(make_pair((*i)->priority(), (*i)->id()));
   }
   // Sort from rbegin to rend in order to get descending priority order.
   std::sort(ordered_matches.rbegin(), ordered_matches.rend());
 
   // Build a map that maps each extension id to the minimum required priority
   // for rules of that extension. Initially, this priority is -infinite and
   // will be increased when the rules are processed and raise the bar via
   // WebRequestIgnoreRulesActions.
   typedef std::string ExtensionId;
   typedef std::map<ExtensionId, WebRequestRule::Priority> MinPriorities;
@@ skipping 72 matching lines @@
        i != new_webrequest_rules.end(); ++i) {
     URLMatcherConditionSet::Vector url_condition_sets;
     const WebRequestConditionSet& conditions = i->second->conditions();
     conditions.GetURLMatcherConditionSets(&url_condition_sets);
     for (URLMatcherConditionSet::Vector::iterator j =
          url_condition_sets.begin(); j != url_condition_sets.end(); ++j) {
       rule_triggers_[(*j)->id()] = i->second.get();
     }
   }
 
-  // Register url patterns in url_matcher_.
+  // Register url patterns in |url_matcher_| and
+  // |rules_with_untriggered_conditions_|.
   URLMatcherConditionSet::Vector all_new_condition_sets;
   for (RulesMap::iterator i = new_webrequest_rules.begin();
        i != new_webrequest_rules.end(); ++i) {
     i->second->conditions().GetURLMatcherConditionSets(&all_new_condition_sets);
+    if (i->second->conditions().HasConditionsWithoutUrls())
+      rules_with_untriggered_conditions_.insert(i->second.get());
   }
   url_matcher_.AddConditionSets(all_new_condition_sets);
 
   ClearCacheOnNavigation();
 
   return "";
 }
 
 std::string WebRequestRulesRegistry::RemoveRulesImpl(
     const std::string& extension_id,
@@ skipping 13 matching lines @@
     // Remove all triggers but collect their IDs.
     URLMatcherConditionSet::Vector condition_sets;
     WebRequestRule* rule = webrequest_rules_entry->second.get();
     rule->conditions().GetURLMatcherConditionSets(&condition_sets);
     for (URLMatcherConditionSet::Vector::iterator j = condition_sets.begin();
          j != condition_sets.end(); ++j) {
       remove_from_url_matcher.push_back((*j)->id());
       rule_triggers_.erase((*j)->id());
     }
 
-    // Remove reference to actual rule.
+    rules_with_untriggered_conditions_.erase(rule);
+
+    // Removes the owning references to (and thus deletes) the rule.
     webrequest_rules_.erase(webrequest_rules_entry);
   }
 
   // Clear URLMatcher based on condition_set_ids that are not needed any more.
   url_matcher_.RemoveConditionSets(remove_from_url_matcher);
 
   ClearCacheOnNavigation();
 
   return "";
 }
@@ skipping 31 matching lines @@
     return base::Time();
 
   return extension_info_map_->GetInstallTime(extension_id);
 }
 
 void WebRequestRulesRegistry::ClearCacheOnNavigation() {
   extension_web_request_api_helpers::ClearCacheOnNavigation();
 }
 
 }  // namespace extensions