Revert of [Extensions Bindings] Avoid passing the event filter to JS (patchset...

Revert of [Extensions Bindings] Avoid passing the event filter to JS (patchset #4 id:140001 of https://codereview.chromium.org/2924683002/ )

Reason for revert:
New test seems to be flaky on Mac...

https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.mac%2FMac10.9_Tests__dbg_%2F41569%2F%2B%2Frecipes%2Fsteps%2Fbrowser_tests%2F0%2Flogs%2FExtensionBindingsApiTest.EventOverriding%2F0

Original issue's description:
> [Extensions Bindings] Avoid passing the event filter to JS
>
> Currently, in JS bindings, we pass the event filter to JS, and then pass
> it back to C++ to find matching event listeners. This can cause problems
> because the object could be tweaked by monkey-patching, and we aren't
> correctly checking.
>
> Instead of updating the checks to safely interact with the object, just
> avoid passing it to JS altogether. Instead, pass the ids of the relevant
> listeners directly to JS. This avoids the security issue and is also
> more performant.
>
> Add a regression test.
>
> BUG=702946
>
> Review-Url: https://codereview.chromium.org/2924683002
> Cr-Commit-Position: refs/heads/master@{#478742}
> Committed: https://chromium.googlesource.com/chromium/src/+/229f3b010af25c9bc38c01cfc7c403d8b5fdecf0

TBR=jbroman@chromium.org,lazyboy@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=702946

Review-Url: https://codereview.chromium.org/2936673004
Cr-Commit-Position: refs/heads/master@{#478832}

chrome/browser/extensions/extension_bindings_apitest.cc

@@ -17,7 +17,6 @@
 #include "extensions/browser/process_manager.h"
 #include "extensions/test/extension_test_message_listener.h"
 #include "extensions/test/result_catcher.h"
-#include "net/dns/mock_host_resolver.h"
 #include "net/test/embedded_test_server/embedded_test_server.h"
 
 namespace extensions {
@@ -27,9 +26,6 @@ class ExtensionBindingsApiTest : public ExtensionApiTest {
  public:
   void SetUpOnMainThread() override {
     ExtensionApiTest::SetUpOnMainThread();
-    host_resolver()->AddRule("*", "127.0.0.1");
-    ASSERT_TRUE(StartEmbeddedTestServer());
-
     content::BrowserThread::PostTask(
         content::BrowserThread::IO, FROM_HERE,
         base::BindOnce(&chrome_browser_net::SetUrlRequestMocksEnabled, true));
@@ -121,6 +117,8 @@ IN_PROC_BROWSER_TEST_F(ExtensionBindingsApiTest, ModuleSystem) {
 }
 
 IN_PROC_BROWSER_TEST_F(ExtensionBindingsApiTest, NoExportOverriding) {
+  ASSERT_TRUE(embedded_test_server()->Start());
+
   // We need to create runtime bindings in the web page. An extension that's
   // externally connectable will do that for us.
   ASSERT_TRUE(LoadExtension(
@@ -143,6 +141,8 @@ IN_PROC_BROWSER_TEST_F(ExtensionBindingsApiTest, NoExportOverriding) {
 }
 
 IN_PROC_BROWSER_TEST_F(ExtensionBindingsApiTest, NoGinDefineOverriding) {
+  ASSERT_TRUE(embedded_test_server()->Start());
+
   // We need to create runtime bindings in the web page. An extension that's
   // externally connectable will do that for us.
   ASSERT_TRUE(LoadExtension(
@@ -167,6 +167,7 @@ IN_PROC_BROWSER_TEST_F(ExtensionBindingsApiTest, NoGinDefineOverriding) {
 }
 
 IN_PROC_BROWSER_TEST_F(ExtensionBindingsApiTest, HandlerFunctionTypeChecking) {
+  ASSERT_TRUE(embedded_test_server()->Start());
   ui_test_utils::NavigateToURL(
       browser(),
       embedded_test_server()->GetURL(
@@ -186,6 +187,8 @@ IN_PROC_BROWSER_TEST_F(ExtensionBindingsApiTest, HandlerFunctionTypeChecking) {
 
 IN_PROC_BROWSER_TEST_F(ExtensionBindingsApiTest,
                        MoreNativeFunctionInterceptionTests) {
+  ASSERT_TRUE(embedded_test_server()->Start());
+
   // We need to create runtime bindings in the web page. An extension that's
   // externally connectable will do that for us.
   ASSERT_TRUE(
@@ -242,6 +245,7 @@ IN_PROC_BROWSER_TEST_F(FramesExtensionBindingsApiTest, FramesBeforeNavigation) {
   // Load the web page which tries to impersonate the sender extension via
   // scripting iframes/child windows before they finish navigating to pages
   // within the sender extension.
+  ASSERT_TRUE(embedded_test_server()->Start());
   ui_test_utils::NavigateToURL(
       browser(),
       embedded_test_server()->GetURL(
@@ -267,6 +271,7 @@ IN_PROC_BROWSER_TEST_F(FramesExtensionBindingsApiTest, FramesBeforeNavigation) {
 }
 
 IN_PROC_BROWSER_TEST_F(ExtensionBindingsApiTest, TestFreezingChrome) {
+  ASSERT_TRUE(embedded_test_server()->Start());
   ui_test_utils::NavigateToURL(
       browser(), embedded_test_server()->GetURL(
                      "/extensions/api_test/bindings/freeze.html"));
@@ -275,18 +280,5 @@ IN_PROC_BROWSER_TEST_F(ExtensionBindingsApiTest, TestFreezingChrome) {
   ASSERT_FALSE(web_contents->IsCrashed());
 }
 
-// Tests interaction with event filter parsing.
-IN_PROC_BROWSER_TEST_F(ExtensionBindingsApiTest, TestEventFilterParsing) {
-  ExtensionTestMessageListener listener("ready", false);
-  ASSERT_TRUE(
-      LoadExtension(test_data_dir_.AppendASCII("bindings/event_filter")));
-  ASSERT_TRUE(listener.WaitUntilSatisfied());
-
-  ResultCatcher catcher;
-  ui_test_utils::NavigateToURL(
-      browser(), embedded_test_server()->GetURL("example.com", "/title1.html"));
-  ASSERT_TRUE(catcher.GetNextResult()) << catcher.message();
-}
-
 }  // namespace
 }  // namespace extensions

chrome/test/data/extensions/api_test/bindings/event_filter/background.js

-// Copyright 2017 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.
-
-// Make sure that putting interceptors on event filter attributes doesn't
-// break anything.
-Object.defineProperty(Object.prototype, 'windowExposedByDefault',
-                      {enumerable: true, get() { return 'hahaha'; }});
-chrome.webNavigation.onBeforeNavigate.addListener(function() {
-  chrome.test.notifyPass();
-}, {url: [{hostContains: 'example.com'}]});
-
-chrome.test.sendMessage('ready');

chrome/test/data/extensions/api_test/bindings/event_filter/manifest.json

-{
-  "name": "event filter fun",
-  "description": "event filter fun",
-  "manifest_version": 2,
-  "version": "0.1",
-  "background": { "scripts": ["background.js"] },
-  "permissions": [ "webNavigation", "*://example.com:*/*" ]
-}

extensions/renderer/event_bindings.cc

@@ -22,7 +22,6 @@
 #include "extensions/common/event_filter.h"
 #include "extensions/common/extension.h"
 #include "extensions/common/extension_messages.h"
-#include "extensions/common/extension_urls.h"
 #include "extensions/common/value_counter.h"
 #include "extensions/renderer/extension_frame_helper.h"
 #include "extensions/renderer/script_context.h"
@@ -89,6 +88,44 @@ int DecrementEventListenerCount(ScriptContext* script_context,
                .Get()[GetKeyForScriptContext(script_context)][event_name];
 }
 
+EventFilteringInfo ParseFromObject(v8::Local<v8::Object> object,
+                                   v8::Isolate* isolate) {
+  EventFilteringInfo info;
+  v8::Local<v8::String> url(v8::String::NewFromUtf8(isolate, "url"));
+  if (object->Has(url)) {
+    v8::Local<v8::Value> url_value(object->Get(url));
+    info.SetURL(GURL(*v8::String::Utf8Value(url_value)));
+  }
+  v8::Local<v8::String> instance_id(
+      v8::String::NewFromUtf8(isolate, "instanceId"));
+  if (object->Has(instance_id)) {
+    v8::Local<v8::Value> instance_id_value(object->Get(instance_id));
+    info.SetInstanceID(instance_id_value->IntegerValue());
+  }
+  v8::Local<v8::String> service_type(
+      v8::String::NewFromUtf8(isolate, "serviceType"));
+  if (object->Has(service_type)) {
+    v8::Local<v8::Value> service_type_value(object->Get(service_type));
+    info.SetServiceType(*v8::String::Utf8Value(service_type_value));
+  }
+  v8::Local<v8::String> window_types(
+      v8::String::NewFromUtf8(isolate, "windowType"));
+  if (object->Has(window_types)) {
+    v8::Local<v8::Value> window_types_value(object->Get(window_types));
+    info.SetWindowType(*v8::String::Utf8Value(window_types_value));
+  }
+
+  v8::Local<v8::String> window_exposed(
+      v8::String::NewFromUtf8(isolate, "windowExposedByDefault"));
+  if (object->Has(window_exposed)) {
+    v8::Local<v8::Value> window_exposed_value(object->Get(window_exposed));
+    info.SetWindowExposedByDefault(
+        window_exposed_value.As<v8::Boolean>()->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,
@@ -126,33 +163,6 @@ bool RemoveFilter(const std::string& event_name,
   return false;
 }
 
-// Returns a v8::Array containing the ids of the listeners that match the given
-// |event_filter_dict| in the given |script_context|.
-v8::Local<v8::Array> GetMatchingListeners(
-    ScriptContext* script_context,
-    const std::string& event_name,
-    const base::DictionaryValue& event_filter_dict) {
-  const EventFilter& event_filter = g_event_filter.Get();
-  EventFilteringInfo info(event_filter_dict);
-  v8::Isolate* isolate = script_context->isolate();
-  v8::Local<v8::Context> context = script_context->v8_context();
-
-  // Only match events routed to this context's RenderFrame or ones that don't
-  // have a routingId in their filter.
-  std::set<EventFilter::MatcherID> matched_event_filters =
-      event_filter.MatchEvent(event_name, info,
-                              script_context->GetRenderFrame()->GetRoutingID());
-  v8::Local<v8::Array> array(
-      v8::Array::New(isolate, matched_event_filters.size()));
-  int i = 0;
-  for (EventFilter::MatcherID id : matched_event_filters) {
-    CHECK(array->CreateDataProperty(context, i++, v8::Integer::New(isolate, id))
-              .ToChecked());
-  }
-
-  return array;
-}
-
 }  // namespace
 
 EventBindings::EventBindings(ScriptContext* context)
@@ -167,6 +177,9 @@ EventBindings::EventBindings(ScriptContext* context)
   RouteFunction("DetachFilteredEvent",
                 base::Bind(&EventBindings::DetachFilteredEventHandler,
                            base::Unretained(this)));
+  RouteFunction("MatchAgainstEventFilter",
+                base::Bind(&EventBindings::MatchAgainstEventFilter,
+                           base::Unretained(this)));
   RouteFunction(
       "AttachUnmanagedEvent",
       base::Bind(&EventBindings::AttachUnmanagedEvent, base::Unretained(this)));
@@ -203,32 +216,6 @@ bool EventBindings::HasListener(ScriptContext* script_context,
   return false;
 }
 
-// static
-void EventBindings::DispatchEventInContext(
-    const std::string& event_name,
-    const base::ListValue* event_args,
-    const base::DictionaryValue* filtering_info,
-    ScriptContext* context) {
-  v8::HandleScope handle_scope(context->isolate());
-  v8::Context::Scope context_scope(context->v8_context());
-
-  v8::Local<v8::Array> listener_ids;
-  if (filtering_info && !filtering_info->empty())
-    listener_ids = GetMatchingListeners(context, event_name, *filtering_info);
-  else
-    listener_ids = v8::Array::New(context->isolate());
-
-  std::unique_ptr<content::V8ValueConverter> converter(
-      content::V8ValueConverter::create());
-  v8::Local<v8::Value> v8_args[] = {
-      gin::StringToSymbol(context->isolate(), event_name),
-      converter->ToV8Value(event_args, context->v8_context()), listener_ids,
-  };
-
-  context->module_system()->CallModuleMethodSafe(
-      kEventBindings, "dispatchEvent", arraysize(v8_args), v8_args);
-}
-
 void EventBindings::AttachEventHandler(
     const v8::FunctionCallbackInfo<v8::Value>& args) {
   CHECK_EQ(1, args.Length());
@@ -308,8 +295,8 @@ void EventBindings::DetachEvent(const std::string& event_name, bool is_manual) {
 // MatcherID AttachFilteredEvent(string event_name, object filter)
 // event_name - Name of the event to attach.
 // filter - Which instances of the named event are we interested in.
-// returns the id assigned to the listener, which will be provided to calls to
-// dispatchEvent().
+// returns the id assigned to the listener, which will be returned from calls
+// to MatchAgainstEventFilter where this listener matches.
 void EventBindings::AttachFilteredEvent(
     const v8::FunctionCallbackInfo<v8::Value>& args) {
   CHECK_EQ(2, args.Length());
@@ -383,6 +370,29 @@ void EventBindings::DetachFilteredEvent(int matcher_id, bool is_manual) {
   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);
+}
+
 void EventBindings::AttachUnmanagedEvent(
     const v8::FunctionCallbackInfo<v8::Value>& args) {
   v8::Isolate* isolate = args.GetIsolate();

extensions/renderer/event_bindings.h

@@ -19,7 +19,6 @@ class Sender;
 
 namespace base {
 class DictionaryValue;
-class ListValue;
 }
 
 namespace extensions {
@@ -36,14 +35,6 @@ class EventBindings : public ObjectBackedNativeHandler {
   static bool HasListener(ScriptContext* context,
                           const std::string& event_name);
 
-  // Dispatches the event in the given |context| with the provided
-  // |event_args| and |filtering_info|.
-  static void DispatchEventInContext(
-      const std::string& event_name,
-      const base::ListValue* event_args,
-      const base::DictionaryValue* filtering_info,
-      ScriptContext* context);
-
  private:
   // JavaScript handler which forwards to AttachEvent().
   // args[0] forwards to |event_name|.
@@ -86,6 +77,8 @@ class EventBindings : public ObjectBackedNativeHandler {
   // listeners are automatically detached.
   void DetachFilteredEvent(int matcher_id, bool is_manual);
 
+  void MatchAgainstEventFilter(const v8::FunctionCallbackInfo<v8::Value>& args);
+
   void AttachUnmanagedEvent(const v8::FunctionCallbackInfo<v8::Value>& args);
   void DetachUnmanagedEvent(const v8::FunctionCallbackInfo<v8::Value>& args);
 

extensions/renderer/js_extension_bindings_system.cc

@@ -28,6 +28,8 @@ namespace extensions {
 
 namespace {
 
+static const char kEventDispatchFunction[] = "dispatchEvent";
+
 // Gets |field| from |object| or creates it as an empty object if it doesn't
 // exist.
 v8::Local<v8::Object> GetOrCreateObject(const v8::Local<v8::Object>& object,
@@ -250,8 +252,26 @@ void JsExtensionBindingsSystem::DispatchEventInContext(
     const base::ListValue* event_args,
     const base::DictionaryValue* filtering_info,
     ScriptContext* context) {
-  EventBindings::DispatchEventInContext(event_name, event_args, filtering_info,
-                                        context);
+  v8::HandleScope handle_scope(context->isolate());
+  v8::Context::Scope context_scope(context->v8_context());
+
+  std::vector<v8::Local<v8::Value>> arguments;
+  arguments.push_back(gin::StringToSymbol(context->isolate(), event_name));
+
+  {
+    std::unique_ptr<content::V8ValueConverter> converter(
+        content::V8ValueConverter::create());
+    arguments.push_back(
+        converter->ToV8Value(event_args, context->v8_context()));
+    if (filtering_info && !filtering_info->empty()) {
+      arguments.push_back(
+          converter->ToV8Value(filtering_info, context->v8_context()));
+    }
+  }
+
+  context->module_system()->CallModuleMethodSafe(
+      kEventBindings, kEventDispatchFunction, arguments.size(),
+      arguments.data());
 }
 
 bool JsExtensionBindingsSystem::HasEventListenerInContext(

extensions/renderer/resources/event.js

@@ -241,15 +241,18 @@
 
   // Dispatches a named event with the given argument array. The args array is
   // the list of arguments that will be sent to the event callback.
-  // |listenerIds| contains the ids of matching listeners, or is an empty array
-  // for all listeners.
-  function dispatchEvent(name, args, listenerIds) {
+  function dispatchEvent(name, args, filteringInfo) {
+    var listenerIDs = [];
+
+    if (filteringInfo)
+      listenerIDs = eventNatives.MatchAgainstEventFilter(name, filteringInfo);
+
     var event = attachedNamedEvents[name];
     if (!event)
       return;
 
     var dispatchArgs = function(args) {
-      var result = event.dispatch_(args, listenerIds);
+      var result = event.dispatch_(args, listenerIDs);
       if (result)
         logging.DCHECK(!result.validationErrors, result.validationErrors);
       return result;