Fix bug in EventFilter that arises when matchers are added to multiple events.

This change fixes a bug in EventFilter to handle the case where the existence of matching EventMatchers for event X causes matching against an event with name Y to crash. It also changes the representation of EventMatcher so that it owns the entire filter dictionary.


BUG=121479

Review URL: https://chromiumcodereview.appspot.com/10534125

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@143142 0039d316-1c4b-4281-b951-d872f2087c98

7 files changed, 130 insertions, 45 deletions

diff --git a/chrome/common/extensions/event_filter.cc b/chrome/common/extensions/event_filter.cc
index e9ccad5..ca23eee 100644
--- a/chrome/common/extensions/event_filter.cc
+++ b/chrome/common/extensions/event_filter.cc
@@ -51,7 +51,7 @@ EventFilter::AddEventMatcher(const std::string& event_name,
                              scoped_ptr<EventMatcher> matcher) {
   MatcherID id = next_id_++;
   URLMatcherConditionSet::Vector condition_sets;
-  if (!CreateConditionSets(id, matcher->url_filters(), &condition_sets))
+  if (!CreateConditionSets(id, matcher.get(), &condition_sets))
     return -1;
 
   for (URLMatcherConditionSet::Vector::iterator it = condition_sets.begin();
@@ -65,19 +65,30 @@ EventFilter::AddEventMatcher(const std::string& event_name,
   return id;
 }
 
+EventMatcher* EventFilter::GetEventMatcher(MatcherID id) {
+  DCHECK(id_to_event_name_.find(id) != id_to_event_name_.end());
+  const std::string& event_name = id_to_event_name_[id];
+  return event_matchers_[event_name][id]->event_matcher();
+}
+
+const std::string& EventFilter::GetEventName(MatcherID id) {
+  DCHECK(id_to_event_name_.find(id) != id_to_event_name_.end());
+  return id_to_event_name_[id];
+}
+
 bool EventFilter::CreateConditionSets(
     MatcherID id,
-    base::ListValue* url_filters,
+    EventMatcher* matcher,
     URLMatcherConditionSet::Vector* condition_sets) {
-  if (!url_filters || url_filters->GetSize() == 0) {
-    // If there are no url_filters then we want to match all events, so create a
+  if (matcher->GetURLFilterCount() == 0) {
+    // If there are no URL filters then we want to match all events, so create a
     // URLFilter from an empty dictionary.
     base::DictionaryValue empty_dict;
     return AddDictionaryAsConditionSet(&empty_dict, condition_sets);
   }
-  for (size_t i = 0; i < url_filters->GetSize(); i++) {
+  for (int i = 0; i < matcher->GetURLFilterCount(); i++) {
     base::DictionaryValue* url_filter;
-    if (!url_filters->GetDictionary(i, &url_filter))
+    if (!matcher->GetURLFilter(i, &url_filter))
       return false;
     if (!AddDictionaryAsConditionSet(url_filter, condition_sets))
       return false;
@@ -106,8 +117,10 @@ bool EventFilter::AddDictionaryAsConditionSet(
 
 std::string EventFilter::RemoveEventMatcher(MatcherID id) {
   std::map<MatcherID, std::string>::iterator it = id_to_event_name_.find(id);
-  event_matchers_[it->second].erase(id);
   std::string event_name = it->second;
+  // EventMatcherEntry's destructor causes the condition set ids to be removed
+  // from url_matcher_.
+  event_matchers_[event_name].erase(id);
   id_to_event_name_.erase(it);
   return event_name;
 }
@@ -126,10 +139,21 @@ std::set<EventFilter::MatcherID> EventFilter::MatchEvent(
   for (std::set<URLMatcherConditionSet::ID>::iterator it =
        matching_condition_set_ids.begin();
        it != matching_condition_set_ids.end(); it++) {
-    MatcherID id = condition_set_id_to_event_matcher_id_[*it];
-    const EventMatcher& event_matcher = matcher_map[id]->event_matcher();
-    if (event_matcher.MatchNonURLCriteria(event_info)) {
-      CHECK(!event_matcher.url_filters() || event_info.has_url());
+    std::map<URLMatcherConditionSet::ID, MatcherID>::iterator matcher_id =
+        condition_set_id_to_event_matcher_id_.find(*it);
+    if (matcher_id == condition_set_id_to_event_matcher_id_.end()) {
+      NOTREACHED() << "id not found in condition set map (" << (*it) << ")";
+      continue;
+    }
+    MatcherID id = matcher_id->second;
+    EventMatcherMap::iterator matcher_entry = matcher_map.find(id);
+    if (matcher_entry == matcher_map.end()) {
+      // Matcher must be for a different event.
+      continue;
+    }
+    const EventMatcher* event_matcher = matcher_entry->second->event_matcher();
+    if (event_matcher->MatchNonURLCriteria(event_info)) {
+      CHECK(!event_matcher->HasURLFilters() || event_info.has_url());
       matchers.insert(id);
     }
   }
diff --git a/chrome/common/extensions/event_filter.h b/chrome/common/extensions/event_filter.h
index 26acb44..6c42c43 100644
--- a/chrome/common/extensions/event_filter.h
+++ b/chrome/common/extensions/event_filter.h
@@ -29,6 +29,14 @@ class EventFilter {
   // the id of the matcher, or -1 if there was an error.
   MatcherID AddEventMatcher(const std::string& event_name,
                             scoped_ptr<EventMatcher> matcher);
+
+  // Retrieve the EventMatcher with the given id.
+  EventMatcher* GetEventMatcher(MatcherID id);
+
+  // Retrieve the name of the event that the EventMatcher specified by |id| is
+  // referring to.
+  const std::string& GetEventName(MatcherID id);
+
   // Removes an event matcher, returning the name of the event that it was for.
   std::string RemoveEventMatcher(MatcherID id);
 
@@ -64,8 +72,8 @@ class EventFilter {
     // and clean them up anyway.
     void DontRemoveConditionSetsInDestructor();
 
-    const EventMatcher& event_matcher() const {
-      return *event_matcher_;
+    EventMatcher* event_matcher() {
+      return event_matcher_.get();
     }
 
    private:
@@ -83,10 +91,10 @@ class EventFilter {
   // Maps from event name to the map of matchers that are registered for it.
   typedef std::map<std::string, EventMatcherMap> EventMatcherMultiMap;
 
-  // Adds the list of filters to the URL matcher, having matches for those URLs
-  // map to |id|.
+  // Adds the list of URL filters in |matcher| to the URL matcher, having
+  // matches for those URLs map to |id|.
   bool CreateConditionSets(MatcherID id,
-                           base::ListValue* url_filters,
+                           EventMatcher* matcher,
                            URLMatcherConditionSet::Vector* condition_sets);
 
   bool AddDictionaryAsConditionSet(
diff --git a/chrome/common/extensions/event_filter_unittest.cc b/chrome/common/extensions/event_filter_unittest.cc
index d1ca5a0..a4ac8ee 100644
--- a/chrome/common/extensions/event_filter_unittest.cc
+++ b/chrome/common/extensions/event_filter_unittest.cc
@@ -34,16 +34,19 @@ class EventFilterUnittest : public testing::Test {
   }
 
   scoped_ptr<EventMatcher> AllURLs() {
-    scoped_ptr<EventMatcher> matcher(new EventMatcher());
-    // An empty set of URL filters always matches.
-    matcher->set_url_filters(scoped_ptr<ListValue>(new ListValue()));
-    return matcher.Pass();
+    return scoped_ptr<EventMatcher>(new EventMatcher(
+        scoped_ptr<DictionaryValue>(new DictionaryValue)));
   }
 
   scoped_ptr<EventMatcher> HostSuffixMatcher(const std::string& host_suffix) {
-    scoped_ptr<EventMatcher> event_matcher(new EventMatcher());
-    event_matcher->set_url_filters(ValueAsList(HostSuffixDict(host_suffix)));
-    return event_matcher.Pass();
+    return MatcherFromURLFilterList(ValueAsList(HostSuffixDict(host_suffix)));
+  }
+
+  scoped_ptr<EventMatcher> MatcherFromURLFilterList(
+      scoped_ptr<ListValue> url_filter_list) {
+    scoped_ptr<DictionaryValue> filter_dict(new DictionaryValue);
+    filter_dict->Set("url", url_filter_list.release());
+    return scoped_ptr<EventMatcher>(new EventMatcher(filter_dict.Pass()));
   }
 
   EventFilter event_filter_;
@@ -121,8 +124,7 @@ TEST_F(EventFilterUnittest, TestMultipleURLFiltersMatchOnAny) {
   filters->Append(HostSuffixDict("google.com").release());
   filters->Append(HostSuffixDict("yahoo.com").release());
 
-  scoped_ptr<EventMatcher> matcher(new EventMatcher());
-  matcher->set_url_filters(filters.Pass());
+  scoped_ptr<EventMatcher> matcher(MatcherFromURLFilterList(filters.Pass()));
   int id = event_filter_.AddEventMatcher("event1", matcher.Pass());
 
   {
@@ -157,6 +159,18 @@ TEST_F(EventFilterUnittest, TestStillMatchesAfterRemoval) {
   }
 }
 
+TEST_F(EventFilterUnittest, TestMatchesOnlyAgainstPatternsForCorrectEvent) {
+  int id1 = event_filter_.AddEventMatcher("event1", AllURLs());
+  event_filter_.AddEventMatcher("event2", AllURLs());
+
+  {
+    std::set<int> matches = event_filter_.MatchEvent("event1",
+        google_event_);
+    ASSERT_EQ(1u, matches.size());
+    ASSERT_EQ(1u, matches.count(id1));
+  }
+}
+
 TEST_F(EventFilterUnittest, TestGetMatcherCountForEvent) {
   ASSERT_EQ(0, event_filter_.GetMatcherCountForEvent("event1"));
   int id1 = event_filter_.AddEventMatcher("event1", AllURLs());
@@ -182,8 +196,8 @@ TEST_F(EventFilterUnittest, RemoveEventMatcherReturnsEventName) {
 TEST_F(EventFilterUnittest, InvalidURLFilterCantBeAdded) {
   scoped_ptr<base::ListValue> filter_list(new base::ListValue());
   filter_list->Append(new base::ListValue());  // Should be a dict.
-  scoped_ptr<EventMatcher> matcher(new EventMatcher);
-  matcher->set_url_filters(filter_list.Pass());
+  scoped_ptr<EventMatcher> matcher(MatcherFromURLFilterList(
+      filter_list.Pass()));
   int id1 = event_filter_.AddEventMatcher("event1", matcher.Pass());
   EXPECT_TRUE(event_filter_.IsURLMatcherEmpty());
   ASSERT_EQ(-1, id1);
@@ -191,8 +205,8 @@ TEST_F(EventFilterUnittest, InvalidURLFilterCantBeAdded) {
 
 TEST_F(EventFilterUnittest, EmptyListOfURLFiltersMatchesAllURLs) {
   scoped_ptr<base::ListValue> filter_list(new base::ListValue());
-  scoped_ptr<EventMatcher> matcher(new EventMatcher);
-  matcher->set_url_filters(filter_list.Pass());
+  scoped_ptr<EventMatcher> matcher(MatcherFromURLFilterList(
+      scoped_ptr<ListValue>(new ListValue)));
   int id = event_filter_.AddEventMatcher("event1", matcher.Pass());
   std::set<int> matches = event_filter_.MatchEvent("event1",
       google_event_);
@@ -219,9 +233,8 @@ TEST_F(EventFilterUnittest, EmptyURLsShouldBeMatchedByEmptyURLFilters) {
 
 TEST_F(EventFilterUnittest,
     EmptyURLsShouldBeMatchedByEmptyURLFiltersWithAnEmptyItem) {
-  scoped_ptr<EventMatcher> matcher(new EventMatcher());
-  matcher->set_url_filters(ValueAsList(scoped_ptr<Value>(
-      new DictionaryValue())));
+  scoped_ptr<EventMatcher> matcher(MatcherFromURLFilterList(ValueAsList(
+      scoped_ptr<Value>(new DictionaryValue()))));
   int id = event_filter_.AddEventMatcher("event1", matcher.Pass());
   std::set<int> matches = event_filter_.MatchEvent("event1", empty_url_event_);
   ASSERT_EQ(1u, matches.size());
diff --git a/chrome/common/extensions/event_filtering_info.cc b/chrome/common/extensions/event_filtering_info.cc
index dcf78fc..1b61399 100644
--- a/chrome/common/extensions/event_filtering_info.cc
+++ b/chrome/common/extensions/event_filtering_info.cc
@@ -4,6 +4,9 @@
 
 #include "chrome/common/extensions/event_filtering_info.h"
 
+#include "base/values.h"
+#include "base/json/json_writer.h"
+
 namespace extensions {
 
 EventFilteringInfo::EventFilteringInfo()
@@ -18,4 +21,14 @@ void EventFilteringInfo::SetURL(const GURL& url) {
   has_url_ = true;
 }
 
+std::string EventFilteringInfo::AsJSONString() const {
+  std::string result;
+  base::DictionaryValue value;
+  if (has_url_)
+    value.SetString("url", url_.spec());
+
+  base::JSONWriter::Write(&value, &result);
+  return result;
+}
+
 }  // namespace extensions
diff --git a/chrome/common/extensions/event_filtering_info.h b/chrome/common/extensions/event_filtering_info.h
index e7077f1..89d4d32 100644
--- a/chrome/common/extensions/event_filtering_info.h
+++ b/chrome/common/extensions/event_filtering_info.h
@@ -27,11 +27,13 @@ class EventFilteringInfo {
   bool has_url() const { return has_url_; }
   const GURL& url() const { return url_; }
 
+  std::string AsJSONString() const;
+
  private:
   bool has_url_;
   GURL url_;
 
-  DISALLOW_COPY_AND_ASSIGN(EventFilteringInfo);
+  // Allow implicit copy and assignment.
 };
 
 }  // namespace extensions
diff --git a/chrome/common/extensions/event_matcher.cc b/chrome/common/extensions/event_matcher.cc
index 1f9e474..8a64f78 100644
--- a/chrome/common/extensions/event_matcher.cc
+++ b/chrome/common/extensions/event_matcher.cc
@@ -5,9 +5,14 @@
 #include "chrome/common/extensions/event_matcher.h"
 #include "chrome/common/extensions/event_filtering_info.h"
 
+namespace {
+const char kUrlFiltersKey[] = "url";
+}
+
 namespace extensions {
 
-EventMatcher::EventMatcher() {
+EventMatcher::EventMatcher(scoped_ptr<base::DictionaryValue> filter)
+    : filter_(filter.Pass()) {
 }
 
 EventMatcher::~EventMatcher() {
@@ -19,4 +24,23 @@ bool EventMatcher::MatchNonURLCriteria(
   return true;
 }
 
+int EventMatcher::GetURLFilterCount() const {
+  base::ListValue* url_filters = NULL;
+  if (filter_->GetList(kUrlFiltersKey, &url_filters))
+    return url_filters->GetSize();
+  return 0;
+}
+
+bool EventMatcher::GetURLFilter(int i, base::DictionaryValue** url_filter_out) {
+  base::ListValue* url_filters = NULL;
+  if (filter_->GetList(kUrlFiltersKey, &url_filters)) {
+    return url_filters->GetDictionary(i, url_filter_out);
+  }
+  return false;
+}
+
+int EventMatcher::HasURLFilters() const {
+  return GetURLFilterCount() != 0;
+}
+
 }  // namespace extensions
diff --git a/chrome/common/extensions/event_matcher.h b/chrome/common/extensions/event_matcher.h
index 7f91962..318777d 100644
--- a/chrome/common/extensions/event_matcher.h
+++ b/chrome/common/extensions/event_matcher.h
@@ -19,28 +19,29 @@ class EventFilteringInfo;
 // MatchNonURLCriteria() - URL matching is handled by EventFilter.
 class EventMatcher {
  public:
-  EventMatcher();
+  explicit EventMatcher(scoped_ptr<base::DictionaryValue> filter);
   ~EventMatcher();
 
   // Returns true if |event_info| satisfies this matcher's criteria, not taking
   // into consideration any URL criteria.
   bool MatchNonURLCriteria(const EventFilteringInfo& event_info) const;
 
-  void set_url_filters(scoped_ptr<base::ListValue> url_filters) {
-    url_filters_ = url_filters.Pass();
-  }
+  int GetURLFilterCount() const;
+  bool GetURLFilter(int i, base::DictionaryValue** url_filter_out);
 
-  // Returns NULL if no url_filters have been specified.
-  base::ListValue* url_filters() const {
-    return url_filters_.get();
-  }
+  int HasURLFilters() const;
 
-  bool has_url_filters() const {
-    return url_filters_.get() && !url_filters_->empty();
+  base::DictionaryValue* value() const {
+    return filter_.get();
   }
 
  private:
-  scoped_ptr<base::ListValue> url_filters_;
+  // Contains a dictionary that corresponds to a single event filter, eg:
+  //
+  // {url: [{hostSuffix: 'google.com'}]}
+  //
+  // The valid filter keys are event-specific.
+  scoped_ptr<base::DictionaryValue> filter_;
 
   DISALLOW_COPY_AND_ASSIGN(EventMatcher);
 };