Ignore filtered event if an event matcher cannot be added.

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 <stdint.h>
 
 #include <map>
 #include <memory>
@@ skipping 256 matching lines @@
         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 = base::DictionaryValue::From(std::move(filter_value));
   }
 
   // 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();

[Devlin, 2016/08/11 19:08:12]

This makes me sad, because there's an awful lot of redirection going on here
where it's easy for this to get lost.  I think it would be a little better
(though still not great) if we kept around a reference to the EventMatcher and
accessed filter through that, rather than a reference to the filter itself - at
least that's one less layer of redirection to keep track of.

[meacer, 2016/08/11 21:53:00]

Done. It seems safe to do this, but I wonder if I should add a CHECK to see if
the previous filter value is equal to the one we get after adding the event
matcher and getting it back. Since this is single threaded JS code, I guess it's
not needed?

[Devlin, 2016/08/11 21:57:39]

I wouldn't be opposed to a DCHECK, if you want to add one.

[meacer, 2016/08/11 22:04:27]

On second thought this seemed overkill so decided not to do it :)

   int id = g_event_filter.Get().AddEventMatcher(
       event_name, ParseEventMatcher(std::move(filter)));
+  if (id == -1)
+    return;

[Devlin, 2016/08/11 19:08:12]

It looks like other places this fails, we return -1 (e.g. line 269).

[meacer, 2016/08/11 21:53:00]

Line 259 doesn't set it to -1. Should I add it there too?

[Devlin, 2016/08/11 21:57:39]

I think line 259 throws an error if it fails, so we should be okay there.

[meacer, 2016/08/11 22:04:27]

Ah, okay didn't read it well.

   attached_matcher_ids_.insert(id);
 
   // Only send IPCs the first time a filter gets added.
   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_weak, lazy));
   }
 
@@ skipping 70 matching lines @@
   // 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