Clear an extension's filtered events when a context is destroyed.

extensions/renderer/event_bindings.cc

Index: extensions/renderer/event_bindings.cc
diff --git a/extensions/renderer/event_bindings.cc b/extensions/renderer/event_bindings.cc
index db5fc23d60b859b1537460feb76c399b67c598eb..e1ed70d4f15f09cf0b69717a62a13e756581c532 100644
--- a/extensions/renderer/event_bindings.cc
+++ b/extensions/renderer/event_bindings.cc
@@ -5,9 +5,11 @@
 #include "extensions/renderer/event_bindings.h"
 
 #include <map>
+#include <utility>
 
 #include "base/basictypes.h"
 #include "base/bind.h"
+#include "base/containers/scoped_ptr_map.h"
 #include "base/lazy_instance.h"
 #include "base/memory/scoped_ptr.h"
 #include "components/crx_file/id_util.h"
@@ -36,19 +38,20 @@ typedef std::map<std::string, int> EventListenerCounts;
 base::LazyInstance<std::map<std::string, EventListenerCounts> >
     g_listener_counts = LAZY_INSTANCE_INITIALIZER;
 
-// A map of event names to a (filter -> count) map. The map is used to keep
-// track of which filters are in effect for which events.
-// We notify the browser about filtered event listeners when we transition
-// between 0 and 1.
-typedef std::map<std::string, linked_ptr<ValueCounter> >
-    FilteredEventListenerCounts;
-
-// A map of extension IDs to filtered listener counts for that extension.
-base::LazyInstance<std::map<std::string, FilteredEventListenerCounts> >
-    g_filtered_listener_counts = LAZY_INSTANCE_INITIALIZER;
+// A map of (extension ID, event name) pairs to the filtered listener counts

[not at google - send to devlin, 2015/07/09 22:10:14]

Having the type:
  map<string<map<string, linked_ptr<ValueCounter>>>>
was making me feel nauseous, so I changed it to:
  ScopedPtrMap<pair<string, string>, ValueCounter>
but, not really related to this change.

+// for that pair. The map is used to keep track of which filters are in effect
+// for which events.  We notify the browser about filtered event listeners when
+// we transition between 0 and 1.
+using FilteredEventListenerKey = std::pair<std::string, std::string>;
+using FilteredEventListenerCounts =
+    base::ScopedPtrMap<FilteredEventListenerKey, scoped_ptr<ValueCounter>>;
+base::LazyInstance<FilteredEventListenerCounts> g_filtered_listener_counts =
+    LAZY_INSTANCE_INITIALIZER;
 
 base::LazyInstance<EventFilter> g_event_filter = LAZY_INSTANCE_INITIALIZER;
 
+// Gets a unique string key identifier for a ScriptContext.
+// TODO(kalman): Just use pointer equality..?

[Devlin, 2015/07/10 17:57:30]

either ellipsis (...) or period (.), not undecided. :P

[not at google - send to devlin, 2015/07/10 20:39:17]

Done.

 std::string GetKeyForScriptContext(ScriptContext* script_context) {
   const std::string& extension_id = script_context->GetExtensionID();
   CHECK(crx_file::id_util::IdIsValid(extension_id) ||
@@ -101,15 +104,16 @@ EventFilteringInfo ParseFromObject(v8::Local<v8::Object> object,
 // was the first filter for that event in that extension.
 bool AddFilter(const std::string& event_name,
                const std::string& extension_id,
-               base::DictionaryValue* filter) {
-  FilteredEventListenerCounts& counts =
-      g_filtered_listener_counts.Get()[extension_id];
-  FilteredEventListenerCounts::iterator it = counts.find(event_name);
-  if (it == counts.end())
-    counts[event_name].reset(new ValueCounter);
-
-  int result = counts[event_name]->Add(*filter);
-  return 1 == result;
+               const base::DictionaryValue& filter) {
+  FilteredEventListenerKey key(extension_id, event_name);
+  FilteredEventListenerCounts::const_iterator counts =
+      g_filtered_listener_counts.Get().find(key);

[Devlin, 2015/07/10 17:57:30]

getting the lazy instance is cheap, but not 100% free.  Hang on to a reference?
(also below)

[not at google - send to devlin, 2015/07/10 20:39:17]

Alright, fine. It does set up a lock I suppose.

+  if (counts == g_filtered_listener_counts.Get().end()) {
+    counts = g_filtered_listener_counts.Get()
+                 .insert(key, make_scoped_ptr(new ValueCounter()))
+                 .first;
+  }
+  return counts->second->Add(filter) == 1;
 }
 
 // Remove a filter from |event_name| in |extension_id|, returning true if it
@@ -117,12 +121,12 @@ bool AddFilter(const std::string& event_name,
 bool RemoveFilter(const std::string& event_name,
                   const std::string& extension_id,
                   base::DictionaryValue* filter) {
-  FilteredEventListenerCounts& counts =
-      g_filtered_listener_counts.Get()[extension_id];
-  FilteredEventListenerCounts::iterator it = counts.find(event_name);
-  if (it == counts.end())
+  FilteredEventListenerKey key(extension_id, event_name);
+  FilteredEventListenerCounts::const_iterator counts =
+      g_filtered_listener_counts.Get().find(key);
+  if (counts == g_filtered_listener_counts.Get().end())
     return false;
-  return 0 == it->second->Remove(*filter);
+  return counts->second->Remove(*filter) == 0;

[Devlin, 2015/07/10 17:57:30]

Won't this be incorrect in the case of trying to remove the last value twice? 
You can fix it by removing the entry in the map after it's empty.  (This was
also a problem before)

[not at google - send to devlin, 2015/07/10 20:39:17]

Done.

 }
 
 }  // namespace
@@ -136,9 +140,9 @@ EventBindings::EventBindings(ScriptContext* context)
   RouteFunction(
       "AttachFilteredEvent",
       base::Bind(&EventBindings::AttachFilteredEvent, base::Unretained(this)));
-  RouteFunction(
-      "DetachFilteredEvent",
-      base::Bind(&EventBindings::DetachFilteredEvent, base::Unretained(this)));
+  RouteFunction("DetachFilteredEvent",
+                base::Bind(&EventBindings::DetachFilteredEventHandler,
+                           base::Unretained(this)));
   RouteFunction("MatchAgainstEventFilter",
                 base::Bind(&EventBindings::MatchAgainstEventFilter,
                            base::Unretained(this)));
@@ -230,56 +234,53 @@ void EventBindings::AttachFilteredEvent(
   if (!context()->HasAccessOrThrowError(event_name))
     return;
 
-  std::string extension_id = context()->GetExtensionID();
-
   scoped_ptr<base::DictionaryValue> filter;
-  scoped_ptr<content::V8ValueConverter> converter(
-      content::V8ValueConverter::create());
-
-  base::DictionaryValue* filter_dict = NULL;
-  base::Value* filter_value = converter->FromV8Value(
-      v8::Local<v8::Object>::Cast(args[1]), context()->v8_context());
-  if (!filter_value) {
-    args.GetReturnValue().Set(static_cast<int32_t>(-1));
-    return;
-  }
-  if (!filter_value->GetAsDictionary(&filter_dict)) {
-    delete filter_value;
-    args.GetReturnValue().Set(static_cast<int32_t>(-1));
-    return;
+  {
+    scoped_ptr<content::V8ValueConverter> converter(
+        content::V8ValueConverter::create());
+    scoped_ptr<base::Value> filter_value(converter->FromV8Value(
+        v8::Local<v8::Object>::Cast(args[1]), context()->v8_context()));
+    if (!filter_value || !filter_value->IsType(base::Value::TYPE_DICTIONARY)) {
+      args.GetReturnValue().Set(static_cast<int32_t>(-1));
+      return;
+    }
+    filter.reset(static_cast<base::DictionaryValue*>(filter_value.release()));
   }
 
-  filter.reset(filter_dict);
-  EventFilter& event_filter = g_event_filter.Get();
-  int id =
-      event_filter.AddEventMatcher(event_name, ParseEventMatcher(filter.get()));
+  // Hold onto a weak reference to |filter| so that it can be used after passing
+  // ownership to |event_filter|.
+  base::DictionaryValue* filter_weak = filter.get();
+  int id = g_event_filter.Get().AddEventMatcher(
+      event_name, ParseEventMatcher(filter.Pass()));
+  attached_matcher_ids_.insert(id);
 
   // Only send IPCs the first time a filter gets added.
-  if (AddFilter(event_name, extension_id, filter.get())) {
+  std::string extension_id = context()->GetExtensionID();
+  if (AddFilter(event_name, extension_id, *filter_weak)) {
     bool lazy = ExtensionFrameHelper::IsContextForEventPage(context());
     content::RenderThread::Get()->Send(new ExtensionHostMsg_AddFilteredListener(
-        extension_id, event_name, *filter, lazy));
+        extension_id, event_name, *filter_weak, lazy));
   }
 
   args.GetReturnValue().Set(static_cast<int32_t>(id));
 }
 
-void EventBindings::DetachFilteredEvent(
+void EventBindings::DetachFilteredEventHandler(
     const v8::FunctionCallbackInfo<v8::Value>& args) {
   CHECK_EQ(2, args.Length());
   CHECK(args[0]->IsInt32());
   CHECK(args[1]->IsBoolean());
-  bool is_manual = args[1]->BooleanValue();
-
-  std::string extension_id = context()->GetExtensionID();
+  DetachFilteredEvent(args[0]->Int32Value(), args[1]->BooleanValue());
+}
 
-  int matcher_id = args[0]->Int32Value();
+void EventBindings::DetachFilteredEvent(int matcher_id, bool is_manual) {
   EventFilter& event_filter = g_event_filter.Get();
   EventMatcher* event_matcher = event_filter.GetEventMatcher(matcher_id);
 
   const std::string& event_name = event_filter.GetEventName(matcher_id);
 
   // Only send IPCs the last time a filter gets removed.
+  std::string extension_id = context()->GetExtensionID();
   if (RemoveFilter(event_name, extension_id, event_matcher->value())) {
     bool remove_lazy =
         is_manual && ExtensionFrameHelper::IsContextForEventPage(context());
@@ -289,6 +290,7 @@ void EventBindings::DetachFilteredEvent(
   }
 
   event_filter.RemoveEventMatcher(matcher_id);
+  attached_matcher_ids_.erase(matcher_id);
 }
 
 void EventBindings::MatchAgainstEventFilter(
@@ -315,10 +317,9 @@ void EventBindings::MatchAgainstEventFilter(
 }
 
 scoped_ptr<EventMatcher> EventBindings::ParseEventMatcher(
-    base::DictionaryValue* filter_dict) {
-  return scoped_ptr<EventMatcher>(new EventMatcher(
-      scoped_ptr<base::DictionaryValue>(filter_dict->DeepCopy()),
-      context()->GetRenderFrame()->GetRoutingID()));
+    scoped_ptr<base::DictionaryValue> filter) {
+  return make_scoped_ptr(new EventMatcher(
+      filter.Pass(), context()->GetRenderFrame()->GetRoutingID()));

[not at google - send to devlin, 2015/07/09 22:10:14]

And again, not strictly related to this change, but having a DeepCopy() here was
silly, so I fixed it.

 }
 
 void EventBindings::OnInvalidated() {
@@ -330,6 +331,14 @@ void EventBindings::OnInvalidated() {
   }
   DCHECK(attached_event_names_.empty())
       << "Events cannot be attached during invalidation";
+
+  // Same for filtered events.
+  std::set<int> attached_matcher_ids_safe = attached_matcher_ids_;
+  for (int matcher_id : attached_matcher_ids_safe) {
+    DetachFilteredEvent(matcher_id, false /* is_manual */);
+  }
+  DCHECK(attached_matcher_ids_.empty())
+      << "Filtered events cannot be attached during invalidation";
 }
 
 }  // namespace extensions