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

extensions/renderer/event_bindings.cc

 // Copyright 2014 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 "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"
 #include "content/public/child/v8_value_converter.h"
 #include "content/public/renderer/render_frame.h"
 #include "content/public/renderer/render_thread.h"
 #include "content/public/renderer/render_view.h"
 #include "extensions/common/event_filter.h"
 #include "extensions/common/extension.h"
 #include "extensions/common/extension_messages.h"
 #include "extensions/common/value_counter.h"
 #include "extensions/renderer/extension_frame_helper.h"
 #include "extensions/renderer/script_context.h"
 #include "url/gurl.h"
 
 namespace extensions {
 
 namespace {
 
 // A map of event names to the number of contexts listening to that event.
 // We notify the browser about event listeners when we transition between 0
 // and 1.
 typedef std::map<std::string, int> EventListenerCounts;
 
 // A map of extension IDs to listener counts for that extension.
 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) ||
         script_context->GetURL().is_valid());
   return crx_file::id_util::IdIsValid(extension_id)
              ? extension_id
              : script_context->GetURL().spec();
 }
 
 // Increments the number of event-listeners for the given |event_name| and
@@ skipping 32 matching lines @@
     v8::Local<v8::Value> service_type_value(object->Get(service_type));
     info.SetServiceType(*v8::String::Utf8Value(service_type_value));
   }
   return info;
 }
 
 // Add a filter to |event_name| in |extension_id|, returning true if it
 // 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
 // was the last filter for that event in that extension.
 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
 
 EventBindings::EventBindings(ScriptContext* context)
     : ObjectBackedNativeHandler(context) {
   RouteFunction("AttachEvent", base::Bind(&EventBindings::AttachEventHandler,
                                           base::Unretained(this)));
   RouteFunction("DetachEvent", base::Bind(&EventBindings::DetachEventHandler,
                                           base::Unretained(this)));
   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)));
 
   // It's safe to use base::Unretained here because |context| will always
   // outlive us.
   context->AddInvalidationObserver(
       base::Bind(&EventBindings::OnInvalidated, base::Unretained(this)));
 }
 
@@ skipping 71 matching lines @@
 void EventBindings::AttachFilteredEvent(
     const v8::FunctionCallbackInfo<v8::Value>& args) {
   CHECK_EQ(2, args.Length());
   CHECK(args[0]->IsString());
   CHECK(args[1]->IsObject());
 
   std::string event_name = *v8::String::Utf8Value(args[0]);
   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();
+  DetachFilteredEvent(args[0]->Int32Value(), args[1]->BooleanValue());
+}
 
-  std::string extension_id = context()->GetExtensionID();
-
-  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());
     content::RenderThread::Get()->Send(
         new ExtensionHostMsg_RemoveFilteredListener(
             extension_id, event_name, *event_matcher->value(), remove_lazy));
   }
 
   event_filter.RemoveEventMatcher(matcher_id);
+  attached_matcher_ids_.erase(matcher_id);
 }
 
 void EventBindings::MatchAgainstEventFilter(
     const v8::FunctionCallbackInfo<v8::Value>& args) {
   v8::Isolate* isolate = args.GetIsolate();
   typedef std::set<EventFilter::MatcherID> MatcherIDs;
   EventFilter& event_filter = g_event_filter.Get();
   std::string event_name = *v8::String::Utf8Value(args[0]);
   EventFilteringInfo info =
       ParseFromObject(args[1]->ToObject(isolate), isolate);
   // Only match events routed to this context's RenderFrame or ones that don't
   // have a routingId in their filter.
   MatcherIDs matched_event_filters = event_filter.MatchEvent(
       event_name, info, context()->GetRenderFrame()->GetRoutingID());
   v8::Local<v8::Array> array(
       v8::Array::New(isolate, matched_event_filters.size()));
   int i = 0;
   for (MatcherIDs::iterator it = matched_event_filters.begin();
        it != matched_event_filters.end();
        ++it) {
     array->Set(v8::Integer::New(isolate, i++), v8::Integer::New(isolate, *it));
   }
   args.GetReturnValue().Set(array);
 }
 
 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() {
   // Detach all attached events that weren't attached. Iterate over a copy
   // because it will be mutated.
   std::set<std::string> attached_event_names_safe = attached_event_names_;
   for (const std::string& event_name : attached_event_names_safe) {
     DetachEvent(event_name, false /* is_manual */);
   }
   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