Fix bugs in filtered event removal code

extensions/browser/event_router_unittest.cc

Index: extensions/browser/event_router_unittest.cc
diff --git a/extensions/browser/event_router_unittest.cc b/extensions/browser/event_router_unittest.cc
index 7fc725f34fca7ed15d21a10ab9e7f13b118e0d38..e977bd0e6d66273f4d0d1d94dfe0791593aead6a 100644
--- a/extensions/browser/event_router_unittest.cc
+++ b/extensions/browser/event_router_unittest.cc
@@ -9,18 +9,29 @@
 #include <utility>
 
 #include "base/bind.h"
-#include "base/compiler_specific.h"
-#include "base/macros.h"
+#include "base/compiler_specific.h" #include "base/macros.h"

[Devlin, 2016/12/02 21:41:24]

typo

[lazyboy, 2016/12/03 00:12:25]

Done.

 #include "base/memory/ptr_util.h"
 #include "base/test/histogram_tester.h"
 #include "base/values.h"
+#include "components/pref_registry/pref_registry_syncable.h"
+#include "components/prefs/pref_service_factory.h"
+#include "components/prefs/testing_pref_store.h"
+#include "content/public/browser/browser_context.h"
+#include "content/public/test/mock_render_process_host.h"
 #include "content/public/test/test_browser_thread_bundle.h"
 #include "extensions/browser/event_listener_map.h"
+#include "extensions/browser/extension_pref_value_map.h"
+#include "extensions/browser/extension_prefs.h"
+#include "extensions/browser/extension_prefs_factory.h"
 #include "extensions/browser/extensions_test.h"
 #include "extensions/common/extension_builder.h"
 #include "extensions/common/test_util.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+using base::DictionaryValue;
+using base::ListValue;
+using base::StringValue;
+
 namespace extensions {
 
 namespace {
@@ -106,6 +117,19 @@ scoped_refptr<Extension> CreateExtension(bool component, bool persistent) {
   return builder.Build();
 }
 
+std::unique_ptr<DictionaryValue> CreateHostSuffixFilter(
+    const std::string& suffix) {
+  std::unique_ptr<DictionaryValue> filter(new DictionaryValue());
+  std::unique_ptr<ListValue> filter_list(new ListValue());
+  std::unique_ptr<DictionaryValue> filter_dict(new DictionaryValue());
+
+  filter_dict->Set("hostSuffix", new StringValue(suffix));

[Devlin, 2016/12/02 21:41:25]

ditto with line 128

[lazyboy, 2016/12/03 00:12:25]

Done.

+  filter_list->Append(std::move(filter_dict));
+  filter->Set("url", filter_list.release());

[Devlin, 2016/12/02 21:41:25]

nit: DictionaryValue::Set() can take a unique_ptr - prefer that one.

[lazyboy, 2016/12/03 00:12:25]

Done.

+
+  return filter;
+}
+
 }  // namespace
 
 class EventRouterTest : public ExtensionsTest {
@@ -163,6 +187,99 @@ class EventRouterTest : public ExtensionsTest {
   DISALLOW_COPY_AND_ASSIGN(EventRouterTest);
 };
 
+class EventRouterFilterTest : public ExtensionsTest {
+ public:
+  EventRouterFilterTest() {}
+
+  void SetUp() override {
+    // extensions::ExtensionServiceTestBase::SetUp();

[Devlin, 2016/12/02 21:41:24]

dd

[lazyboy, 2016/12/03 00:12:25]

Done.

+    ExtensionsTest::SetUp();
+
+    render_process_host_.reset(
+        new content::MockRenderProcessHost(browser_context()));
+
+    // Set up all the dependencies of ExtensionPrefs.
+    extension_pref_value_map_.reset(new ExtensionPrefValueMap());
+    PrefServiceFactory factory;
+    factory.set_user_prefs(new TestingPrefStore());
+    factory.set_extension_prefs(new TestingPrefStore());
+    user_prefs::PrefRegistrySyncable* pref_registry =
+        new user_prefs::PrefRegistrySyncable();
+    // Prefs should be registered before the PrefService is created.
+    ExtensionPrefs::RegisterProfilePrefs(pref_registry);
+    pref_service_ = factory.Create(pref_registry);
+
+    std::unique_ptr<ExtensionPrefs> extension_prefs(ExtensionPrefs::Create(
+        browser_context(), pref_service_.get(),
+        browser_context()->GetPath().AppendASCII("Extensions"),
+        extension_pref_value_map_.get(), false /* extensions_disabled */,
+        std::vector<ExtensionPrefsObserver*>()));
+
+    ExtensionPrefsFactory::GetInstance()->SetInstanceForTesting(
+        browser_context(), std::move(extension_prefs));
+
+    ASSERT_TRUE(EventRouter::Get(browser_context()));  // constructs EventRouter
+  }
+
+  void TearDown() override {
+    render_process_host_.reset();
+    ExtensionsTest::TearDown();
+  }
+
+  content::RenderProcessHost* render_process_host() const {
+    return render_process_host_.get();
+  }
+
+  EventRouter* event_router() { return EventRouter::Get(browser_context()); }
+
+  const DictionaryValue* GetFilteredEvents(const std::string& extension_id) {
+    return event_router()->GetFilteredEvents(extension_id);
+  }
+
+  bool ContainsFilter(const std::string& extension_id,
+                      const std::string& event_name,
+                      const DictionaryValue& to_check) {
+    const ListValue* filter_list = GetFilterList(extension_id, event_name);
+    if (!filter_list) {
+      ADD_FAILURE();
+      return false;
+    }
+
+    for (size_t i = 0; i < filter_list->GetSize(); ++i) {
+      const DictionaryValue* filter = nullptr;
+      if (!filter_list->GetDictionary(i, &filter)) {
+        ADD_FAILURE();
+        return false;
+      }
+      if (filter->Equals(&to_check))
+        return true;
+    }
+    return false;
+  }
+
+ private:
+  const ListValue* GetFilterList(const std::string& extension_id,
+                                 const std::string& event_name) {
+    const base::DictionaryValue* filtered_events =
+        GetFilteredEvents(extension_id);
+    DictionaryValue::Iterator iter(*filtered_events);
+    if (iter.key() != event_name)

[Devlin, 2016/12/02 21:41:25]

This confuses me.  Why do we only check one entry?  Why not use
DictionaryValue::GetList?

[lazyboy, 2016/12/03 00:12:25]

The pref dictionary value looks like this:
{
  "onEventFoo" : [ filter1, filter2, ..],
  "onEventBar" : [ filterA, filterB, ...]
}

Since we only wrote only one event, there's only one entry. 

+      return nullptr;
+
+    const base::ListValue* filter_list = nullptr;
+    if (!iter.value().GetAsList(&filter_list))
+      return nullptr;

[Devlin, 2016/12/02 21:41:24]

assuming we keep this impl, we could just ignore the result of the GetAsList()
and return filter_list, which will be null if it failed.

[lazyboy, 2016/12/03 00:12:25]

Done.

+
+    return filter_list;
+  }
+
+  std::unique_ptr<content::RenderProcessHost> render_process_host_;
+  std::unique_ptr<ExtensionPrefValueMap> extension_pref_value_map_;
+  std::unique_ptr<PrefService> pref_service_;
+
+  DISALLOW_COPY_AND_ASSIGN(EventRouterFilterTest);
+};
+
 TEST_F(EventRouterTest, GetBaseEventName) {
   // Normal event names are passed through unchanged.
   EXPECT_EQ("foo.onBar", EventRouter::GetBaseEventName("foo.onBar"));
@@ -275,4 +392,60 @@ TEST_F(EventRouterTest, TestReportEvent) {
   ExpectHistogramCounts(7, 3, 2, 2, 1, 2);
 }
 
+// Tests adding and removing events with filters.
+TEST_F(EventRouterFilterTest, Basic) {
+  // For the purpose of this test, "." is important in |event_name| as it
+  // exercises the code path that uses |event_name| as a key in DictionaryValue.
+  const std::string event_name = "chrome.onMyEvent";

[Devlin, 2016/12/02 21:41:24]

nit: prefer kEventName, kExtensionId, etc for constants.

[Devlin, 2016/12/02 21:41:24]

Just for test clarity, it might be better to use a real event name (or one
closer to it).  We don't include the "chrome" normally, so this might confuse
readers as to what it's actually testing.

[lazyboy, 2016/12/03 00:12:25]

Done.

[lazyboy, 2016/12/03 00:12:25]

Done.

+
+  const std::string extension_id = "mbflcebpggnecokmikipoihdbecnjfoj";
+  std::string host_suffixes[] = {"foo.com", "bar.com", "baz.com"};
+  std::vector<std::unique_ptr<DictionaryValue>> filters;
+  for (size_t i = 0; i < arraysize(host_suffixes); ++i) {
+    std::unique_ptr<base::DictionaryValue> filter1 =
+        CreateHostSuffixFilter(host_suffixes[i]);
+    EventRouter::Get(browser_context())
+        ->AddFilteredEventListener(event_name, render_process_host(),
+                                   extension_id, *filter1.get(), true);

[Devlin, 2016/12/02 21:41:25]

*foo.get() is redundant; -> *foo.

[lazyboy, 2016/12/03 00:12:25]

Done.

+    filters.push_back(std::move(filter1));
+  }
+
+  const base::DictionaryValue* filtered_events =
+      GetFilteredEvents(extension_id);
+  ASSERT_TRUE(filtered_events);
+  ASSERT_EQ(1u, filtered_events->size());
+
+  DictionaryValue::Iterator iter(*filtered_events);
+  ASSERT_EQ(event_name, iter.key());
+  const base::ListValue* filter_list = nullptr;
+  ASSERT_TRUE(iter.value().GetAsList(&filter_list));
+  ASSERT_TRUE(filter_list);
+  ASSERT_EQ(3u, filter_list->GetSize());
+
+  ASSERT_TRUE(ContainsFilter(extension_id, event_name, *filters[0].get()));
+  ASSERT_TRUE(ContainsFilter(extension_id, event_name, *filters[1].get()));
+  ASSERT_TRUE(ContainsFilter(extension_id, event_name, *filters[2].get()));
+
+  // Remove the second filter.
+  event_router()->RemoveFilteredEventListener(
+      event_name, render_process_host(), extension_id, *filters[1].get(), true);
+  ASSERT_TRUE(ContainsFilter(extension_id, event_name, *filters[0].get()));
+  ASSERT_FALSE(ContainsFilter(extension_id, event_name, *filters[1].get()));
+  ASSERT_TRUE(ContainsFilter(extension_id, event_name, *filters[2].get()));
+
+  // Remove the first filter.
+  event_router()->RemoveFilteredEventListener(
+      event_name, render_process_host(), extension_id, *filters[0].get(), true);
+  ASSERT_FALSE(ContainsFilter(extension_id, event_name, *filters[0].get()));
+  ASSERT_FALSE(ContainsFilter(extension_id, event_name, *filters[1].get()));
+  ASSERT_TRUE(ContainsFilter(extension_id, event_name, *filters[2].get()));
+
+  // Remove the third filter.
+  event_router()->RemoveFilteredEventListener(
+      event_name, render_process_host(), extension_id, *filters[2].get(), true);
+  ASSERT_FALSE(ContainsFilter(extension_id, event_name, *filters[0].get()));
+  ASSERT_FALSE(ContainsFilter(extension_id, event_name, *filters[1].get()));
+  ASSERT_FALSE(ContainsFilter(extension_id, event_name, *filters[2].get()));
+}
+
 }  // namespace extensions