[Extensions Bindings] Speculative fix for GetContextOwner() crash

We're seeing crashes in GetContextOwner(), which is used to return an
identifier for a v8::Context that is constant across multiple contexts
for that same owner (e.g., an extension id). The crash is happening
at a failed CHECK:

ScriptContext* script_context = GetScriptContextFromV8ContextChecked(context);
const std::string& extension_id = script_context->GetExtensionID();
bool id_is_valid = crx_file::id_util::IdIsValid(extension_id);
CHECK(id_is_valid || script_context->url().is_valid());

Looking at the crash dumps, it appears that this crash happens with a
ScriptContext that has neither an associated extension nor a valid URL
(the URL is empty). The context type is for a Feature::WEB_PAGE_CONTEXT.

In theory, this should never happen, because contexts should only be
classified as a WEB_PAGE_CONTEXT with a valid URL. However, there is a
chance that it could, as described in https://crbug.com/877658. My
suspicion is that this happens during page navigation, when we are
loading/initializing a new context.

Both native and JS-based bindings use the same check for validity, but
it appears this crash only happens in native bindings. The primary
difference is that native bindings check a context owner's identity
immediately on event construction, whereas JS bindings do it lazily on
listener addition/removal.

In order to try and mitigate the crashes, change native bindings to
mimic JS bindings behavior, where the context identity is determined
lazily. This will not fix the underlying context issue, but should
hopefully address the crashes. If it doesn't, we'll have to go back to
the drawing board.

Bug: 877401

Change-Id: I8b29ec872b1fdb33f74b96bfa85e721b5e26b870
Reviewed-on: https://chromium-review.googlesource.com/1188764
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#586092}

extensions/renderer/bindings/api_binding_unittest.cc

@@ -199,7 +199,7 @@ class APIBindingUnittest : public APIBindingTest {
     if (!availability_callback_)
       availability_callback_ = base::Bind(&AllowAllFeatures);
     auto get_context_owner = [](v8::Local<v8::Context>) {
-      return std::string();
+      return std::string("context");
     };
     event_handler_ = std::make_unique<APIEventHandler>(
         base::Bind(&OnEventListenersChanged),

extensions/renderer/bindings/api_bindings_system_unittest.cc

@@ -112,7 +112,9 @@ void APIBindingsSystemTest::SetUp() {
 
   binding::AddConsoleError add_console_error(base::Bind(
       &APIBindingsSystemTest::AddConsoleError, base::Unretained(this)));
-  auto get_context_owner = [](v8::Local<v8::Context>) { return std::string(); };
+  auto get_context_owner = [](v8::Local<v8::Context>) {
+    return std::string("context");
+  };
   bindings_system_ = std::make_unique<APIBindingsSystem>(
       base::Bind(&APIBindingsSystemTest::GetAPISchema, base::Unretained(this)),
       base::Bind(&AllowAllAPIs),

extensions/renderer/bindings/api_event_handler.cc

@@ -99,7 +99,7 @@ void DispatchEvent(const v8::FunctionCallbackInfo<v8::Value>& info) {
 
 APIEventHandler::APIEventHandler(
     const APIEventListeners::ListenersUpdated& listeners_changed,
-    const ContextOwnerIdGetter& context_owner_id_getter,
+    const APIEventListeners::ContextOwnerIdGetter& context_owner_id_getter,
     ExceptionHandler* exception_handler)
     : listeners_changed_(listeners_changed),
       context_owner_id_getter_(context_owner_id_getter),
@@ -119,8 +119,6 @@ v8::Local<v8::Object> APIEventHandler::CreateEventInstance(
   // context directly.
   v8::Context::Scope context_scope(context);
 
-  std::string context_owner = context_owner_id_getter_.Run(context);
-
   APIEventPerContextData* data =
       APIEventPerContextData::GetFrom(context, kCreateIfMissing);
   DCHECK(data->emitters.find(event_name) == data->emitters.end());
@@ -130,11 +128,11 @@ v8::Local<v8::Object> APIEventHandler::CreateEventInstance(
   std::unique_ptr<APIEventListeners> listeners;
   if (supports_filters) {
     listeners = std::make_unique<FilteredEventListeners>(
-        updated, event_name, context_owner, max_listeners,
+        updated, event_name, context_owner_id_getter_, max_listeners,
         supports_lazy_listeners, &listener_tracker_);
   } else {
     listeners = std::make_unique<UnfilteredEventListeners>(
-        updated, event_name, context_owner, max_listeners,
+        updated, event_name, context_owner_id_getter_, max_listeners,
         supports_lazy_listeners, &listener_tracker_);
   }
 
@@ -162,13 +160,13 @@ v8::Local<v8::Object> APIEventHandler::CreateAnonymousEventInstance(
 
   // Anonymous events are not tracked, and thus don't need a name or a context
   // owner.
-  std::string empty_context_owner;
   std::string empty_event_name;
   ListenerTracker* anonymous_listener_tracker = nullptr;
   std::unique_ptr<APIEventListeners> listeners =
       std::make_unique<UnfilteredEventListeners>(
-          base::DoNothing(), empty_context_owner, empty_event_name,
-          binding::kNoListenerMax, false, anonymous_listener_tracker);
+          base::DoNothing(), empty_event_name,
+          APIEventListeners::ContextOwnerIdGetter(), binding::kNoListenerMax,
+          false, anonymous_listener_tracker);
   gin::Handle<EventEmitter> emitter_handle =
       gin::CreateHandle(context->GetIsolate(),
                         new EventEmitter(supports_filters, std::move(listeners),

extensions/renderer/bindings/api_event_handler_unittest.cc

@@ -35,7 +35,7 @@ using MockEventChangeHandler = ::testing::StrictMock<
     base::MockCallback<APIEventListeners::ListenersUpdated>>;
 
 std::string GetContextOwner(v8::Local<v8::Context> context) {
-  return std::string();
+  return "context";
 }
 
 // TODO(devlin): Use these handy functions more places.

extensions/renderer/bindings/api_event_listeners.cc

@@ -79,17 +79,20 @@ bool ValidateFilter(v8::Local<v8::Context> context,
 UnfilteredEventListeners::UnfilteredEventListeners(
     const ListenersUpdated& listeners_updated,
     const std::string& event_name,
-    const std::string& context_owner_id,
+    const ContextOwnerIdGetter& context_owner_id_getter,
     int max_listeners,
     bool supports_lazy_listeners,
     ListenerTracker* listener_tracker)
     : listeners_updated_(listeners_updated),
       event_name_(event_name),
-      context_owner_id_(context_owner_id),
+      context_owner_id_getter_(context_owner_id_getter),
       max_listeners_(max_listeners),
       supports_lazy_listeners_(supports_lazy_listeners),
       listener_tracker_(listener_tracker) {
   DCHECK(max_listeners_ == binding::kNoListenerMax || max_listeners_ > 0);
+  DCHECK_EQ(listener_tracker_ == nullptr, context_owner_id_getter_.is_null())
+      << "Managed events must have both a listener tracker and context owner; "
+      << "unmanaged must have neither.";
 }
 UnfilteredEventListeners::~UnfilteredEventListeners() = default;
 
@@ -116,6 +119,7 @@ bool UnfilteredEventListeners::AddListener(v8::Local<v8::Function> listener,
     // NOTE: |listener_tracker_| is null for unmanaged events, in which case we
     // send no notifications.
     if (listener_tracker_) {
+      LazilySetContextOwner(context);
       bool was_first_listener_for_context_owner =
           listener_tracker_->AddUnfilteredListener(context_owner_id_,
                                                    event_name_);
@@ -175,6 +179,15 @@ void UnfilteredEventListeners::Invalidate(v8::Local<v8::Context> context) {
   }
 }
 
+void UnfilteredEventListeners::LazilySetContextOwner(
+    v8::Local<v8::Context> context) {
+  if (context_owner_id_.empty()) {
+    DCHECK(context_owner_id_getter_);
+    context_owner_id_ = context_owner_id_getter_.Run(context);
+    DCHECK(!context_owner_id_.empty());
+  }
+}
+
 void UnfilteredEventListeners::NotifyListenersEmpty(
     v8::Local<v8::Context> context,
     bool update_lazy_listeners) {
@@ -184,6 +197,9 @@ void UnfilteredEventListeners::NotifyListenersEmpty(
   if (!listener_tracker_)
     return;
 
+  DCHECK(!context_owner_id_.empty())
+      << "The context owner must be instantiated if listeners were removed.";
+
   bool was_last_listener_for_context_owner =
       listener_tracker_->RemoveUnfilteredListener(context_owner_id_,
                                                   event_name_);
@@ -213,16 +229,20 @@ struct FilteredEventListeners::ListenerData {
 FilteredEventListeners::FilteredEventListeners(
     const ListenersUpdated& listeners_updated,
     const std::string& event_name,
-    const std::string& context_owner_id,
+    const ContextOwnerIdGetter& context_owner_id_getter,
     int max_listeners,
     bool supports_lazy_listeners,
     ListenerTracker* listener_tracker)
     : listeners_updated_(listeners_updated),
       event_name_(event_name),
-      context_owner_id_(context_owner_id),
+      context_owner_id_getter_(context_owner_id_getter),
       max_listeners_(max_listeners),
       supports_lazy_listeners_(supports_lazy_listeners),
-      listener_tracker_(listener_tracker) {}
+      listener_tracker_(listener_tracker) {
+  DCHECK(listener_tracker_);
+  DCHECK(context_owner_id_getter_);
+}
+
 FilteredEventListeners::~FilteredEventListeners() = default;
 
 bool FilteredEventListeners::AddListener(v8::Local<v8::Function> listener,
@@ -245,6 +265,7 @@ bool FilteredEventListeners::AddListener(v8::Local<v8::Function> listener,
   base::DictionaryValue* filter_weak = filter_dict.get();
   int filter_id = -1;
   bool was_first_of_kind = false;
+  LazilySetContextOwner(context);
   std::tie(was_first_of_kind, filter_id) =
       listener_tracker_->AddFilteredListener(context_owner_id_, event_name_,
                                              std::move(filter_dict),
@@ -309,10 +330,22 @@ void FilteredEventListeners::Invalidate(v8::Local<v8::Context> context) {
   listeners_.clear();
 }
 
+void FilteredEventListeners::LazilySetContextOwner(
+    v8::Local<v8::Context> context) {
+  if (context_owner_id_.empty()) {
+    DCHECK(context_owner_id_getter_);
+    context_owner_id_ = context_owner_id_getter_.Run(context);
+    DCHECK(!context_owner_id_.empty());
+  }
+}
+
 void FilteredEventListeners::InvalidateListener(
     const ListenerData& listener,
     bool was_manual,
     v8::Local<v8::Context> context) {
+  DCHECK(!context_owner_id_.empty())
+      << "The context owner must be instantiated if listeners were removed.";
+
   bool was_last_of_kind = false;
   std::unique_ptr<base::DictionaryValue> filter;
   std::tie(was_last_of_kind, filter) =

extensions/renderer/bindings/api_event_listeners.h

@@ -38,6 +38,17 @@ class APIEventListeners {
                           bool update_lazy_listeners,
                           v8::Local<v8::Context> context)>;
 
+  // A callback to retrieve the identity of the context's owner. This allows us
+  // to associate multiple listeners from different v8::Contexts with the same
+  // owner (e.g., extension). This is used lazily, when listeners are first
+  // added.
+  // TODO(https://crbug.com/877658): Ideally, we'd just pass in the context
+  // owner to the event directly. However, this led to https://crbug.com/877401,
+  // presumably because of https://crbug.com/877658. If we can fix that, we can
+  // simplify this again.
+  using ContextOwnerIdGetter =
+      base::RepeatingCallback<std::string(v8::Local<v8::Context>)>;
+
   virtual ~APIEventListeners() = default;
 
   // Adds the given |listener| to the list, possibly associating it with the
@@ -81,7 +92,7 @@ class UnfilteredEventListeners final : public APIEventListeners {
  public:
   UnfilteredEventListeners(const ListenersUpdated& listeners_updated,
                            const std::string& event_name,
-                           const std::string& context_owner_id,
+                           const ContextOwnerIdGetter& context_owner_id_getter,
                            int max_listeners,
                            bool supports_lazy_listeners,
                            ListenerTracker* listener_tracker);
@@ -101,6 +112,9 @@ class UnfilteredEventListeners final : public APIEventListeners {
   void Invalidate(v8::Local<v8::Context> context) override;
 
  private:
+  // Lazily sets |context_id_owner_| from |context_id_owner_getter_|.
+  void LazilySetContextOwner(v8::Local<v8::Context> context);
+
   // Notifies that all the listeners for this context are now removed.
   void NotifyListenersEmpty(v8::Local<v8::Context> context,
                             bool update_lazy_listeners);
@@ -120,7 +134,13 @@ class UnfilteredEventListeners final : public APIEventListeners {
   // This event's name.
   std::string event_name_;
 
-  // The owner of the context these listeners belong to.
+  // The getter for the owner of the context; called lazily when the first
+  // listener is added. Only used when the event is managed (i.e.,
+  // |listener_tracker_ is non-null|).
+  ContextOwnerIdGetter context_owner_id_getter_;
+
+  // The owner of the context these listeners belong to. Only used when the
+  // event is managed (i.e., |listener_tracker_ is non-null|).
   std::string context_owner_id_;
 
   // The maximum number of supported listeners.
@@ -145,7 +165,7 @@ class FilteredEventListeners final : public APIEventListeners {
  public:
   FilteredEventListeners(const ListenersUpdated& listeners_updated,
                          const std::string& event_name,
-                         const std::string& context_owner_id,
+                         const ContextOwnerIdGetter& context_owner_id_getter,
                          int max_listeners,
                          bool supports_lazy_listeners,
                          ListenerTracker* listener_tracker);
@@ -167,6 +187,9 @@ class FilteredEventListeners final : public APIEventListeners {
  private:
   struct ListenerData;
 
+  // Lazily sets |context_owner_id_| from |context_owner_id_getter_|.
+  void LazilySetContextOwner(v8::Local<v8::Context> context);
+
   void InvalidateListener(const ListenerData& listener,
                           bool was_manual,
                           v8::Local<v8::Context> context);
@@ -179,7 +202,12 @@ class FilteredEventListeners final : public APIEventListeners {
   // This event's name.
   std::string event_name_;
 
-  // The owner of the context these listeners belong to.
+  // The getter for the owner of the context; called lazily when the first
+  // listener is added. This is always non-null.
+  ContextOwnerIdGetter context_owner_id_getter_;
+
+  // The owner of the context these listeners belong to. This should always be
+  // non-empty after initialization.
   std::string context_owner_id_;
 
   // The maximum number of supported listeners.
@@ -189,7 +217,7 @@ class FilteredEventListeners final : public APIEventListeners {
   bool supports_lazy_listeners_;
 
   // The listener tracker to notify of added or removed listeners. Required to
-  // outlive this object.
+  // outlive this object. Must be non-null.
   ListenerTracker* listener_tracker_ = nullptr;
 
   DISALLOW_COPY_AND_ASSIGN(FilteredEventListeners);

extensions/renderer/bindings/api_event_listeners_unittest.cc

@@ -26,6 +26,11 @@ const char kFunction[] = "(function() {})";
 const char kEvent[] = "event";
 const char kContextOwner[] = "context";
 
+APIEventListeners::ContextOwnerIdGetter CreateContextOwnerIdGetter() {
+  return base::BindRepeating(
+      [](v8::Local<v8::Context>) { return std::string(kContextOwner); });
+}
+
 }  // namespace
 
 // Test unfiltered listeners.
@@ -35,7 +40,8 @@ TEST_F(APIEventListenersTest, UnfilteredListeners) {
 
   MockEventChangeHandler handler;
   ListenerTracker tracker;
-  UnfilteredEventListeners listeners(handler.Get(), kEvent, kContextOwner,
+  UnfilteredEventListeners listeners(handler.Get(), kEvent,
+                                     CreateContextOwnerIdGetter(),
                                      binding::kNoListenerMax, true, &tracker);
 
   // Starting out, there should be no listeners.
@@ -112,7 +118,8 @@ TEST_F(APIEventListenersTest, UnfilteredListenersInvalidation) {
 
   MockEventChangeHandler handler;
   ListenerTracker tracker;
-  UnfilteredEventListeners listeners(handler.Get(), kEvent, kContextOwner,
+  UnfilteredEventListeners listeners(handler.Get(), kEvent,
+                                     CreateContextOwnerIdGetter(),
                                      binding::kNoListenerMax, true, &tracker);
 
   listeners.Invalidate(context);
@@ -145,7 +152,8 @@ TEST_F(APIEventListenersTest, UnfilteredListenersIgnoreFilteringInfo) {
   v8::Local<v8::Context> context = MainContext();
 
   ListenerTracker tracker;
-  UnfilteredEventListeners listeners(base::DoNothing(), kEvent, kContextOwner,
+  UnfilteredEventListeners listeners(base::DoNothing(), kEvent,
+                                     CreateContextOwnerIdGetter(),
                                      binding::kNoListenerMax, true, &tracker);
   v8::Local<v8::Function> function = FunctionFromString(context, kFunction);
   std::string error;
@@ -162,8 +170,9 @@ TEST_F(APIEventListenersTest, UnfilteredListenersMaxListenersTest) {
   v8::Local<v8::Context> context = MainContext();
 
   ListenerTracker tracker;
-  UnfilteredEventListeners listeners(base::DoNothing(), kEvent, kContextOwner,
-                                     1, true, &tracker);
+  UnfilteredEventListeners listeners(base::DoNothing(), kEvent,
+                                     CreateContextOwnerIdGetter(), 1, true,
+                                     &tracker);
 
   v8::Local<v8::Function> function_a = FunctionFromString(context, kFunction);
   EXPECT_EQ(0u, listeners.GetNumListeners());
@@ -188,7 +197,8 @@ TEST_F(APIEventListenersTest, UnfilteredListenersLazyListeners) {
 
   ListenerTracker tracker;
   MockEventChangeHandler handler;
-  UnfilteredEventListeners listeners(handler.Get(), kEvent, kContextOwner,
+  UnfilteredEventListeners listeners(handler.Get(), kEvent,
+                                     CreateContextOwnerIdGetter(),
                                      binding::kNoListenerMax, false, &tracker);
 
   v8::Local<v8::Function> listener = FunctionFromString(context, kFunction);
@@ -215,7 +225,8 @@ TEST_F(APIEventListenersTest, FilteredListeners) {
 
   MockEventChangeHandler handler;
   ListenerTracker tracker;
-  FilteredEventListeners listeners(handler.Get(), kEvent, kContextOwner,
+  FilteredEventListeners listeners(handler.Get(), kEvent,
+                                   CreateContextOwnerIdGetter(),
                                    binding::kNoListenerMax, true, &tracker);
 
   // Starting out, there should be no listeners registered.
@@ -361,7 +372,8 @@ TEST_F(APIEventListenersTest,
 
   MockEventChangeHandler handler;
   ListenerTracker tracker;
-  FilteredEventListeners listeners(handler.Get(), kEvent, kContextOwner,
+  FilteredEventListeners listeners(handler.Get(), kEvent,
+                                   CreateContextOwnerIdGetter(),
                                    binding::kNoListenerMax, true, &tracker);
 
   auto get_filter = [context]() {
@@ -417,7 +429,8 @@ TEST_F(APIEventListenersTest, UnfilteredListenersError) {
   v8::Local<v8::Context> context = MainContext();
 
   ListenerTracker tracker;
-  FilteredEventListeners listeners(base::DoNothing(), kEvent, kContextOwner,
+  FilteredEventListeners listeners(base::DoNothing(), kEvent,
+                                   CreateContextOwnerIdGetter(),
                                    binding::kNoListenerMax, true, &tracker);
 
   v8::Local<v8::Object> invalid_filter =
@@ -440,9 +453,11 @@ TEST_F(APIEventListenersTest, MultipleUnfilteredListenerEvents) {
   const char kBeta[] = "beta";
 
   ListenerTracker tracker;
-  FilteredEventListeners listeners_a(base::DoNothing(), kAlpha, kContextOwner,
+  FilteredEventListeners listeners_a(base::DoNothing(), kAlpha,
+                                     CreateContextOwnerIdGetter(),
                                      binding::kNoListenerMax, true, &tracker);
-  FilteredEventListeners listeners_b(base::DoNothing(), kBeta, kContextOwner,
+  FilteredEventListeners listeners_b(base::DoNothing(), kBeta,
+                                     CreateContextOwnerIdGetter(),
                                      binding::kNoListenerMax, true, &tracker);
 
   EXPECT_EQ(
@@ -497,7 +512,8 @@ TEST_F(APIEventListenersTest, FilteredListenersInvalidation) {
 
   MockEventChangeHandler handler;
   ListenerTracker tracker;
-  FilteredEventListeners listeners(handler.Get(), kEvent, kContextOwner,
+  FilteredEventListeners listeners(handler.Get(), kEvent,
+                                   CreateContextOwnerIdGetter(),
                                    binding::kNoListenerMax, true, &tracker);
   listeners.Invalidate(context);
 
@@ -546,8 +562,9 @@ TEST_F(APIEventListenersTest, FilteredListenersMaxListenersTest) {
   v8::Local<v8::Context> context = MainContext();
 
   ListenerTracker tracker;
-  FilteredEventListeners listeners(base::DoNothing(), kEvent, kContextOwner, 1,
-                                   true, &tracker);
+  FilteredEventListeners listeners(base::DoNothing(), kEvent,
+                                   CreateContextOwnerIdGetter(), 1, true,
+                                   &tracker);
 
   v8::Local<v8::Function> function_a = FunctionFromString(context, kFunction);
   EXPECT_EQ(0u, listeners.GetNumListeners());
@@ -572,7 +589,8 @@ TEST_F(APIEventListenersTest, FilteredListenersLazyListeners) {
 
   MockEventChangeHandler handler;
   ListenerTracker tracker;
-  FilteredEventListeners listeners(handler.Get(), kEvent, kContextOwner,
+  FilteredEventListeners listeners(handler.Get(), kEvent,
+                                   CreateContextOwnerIdGetter(),
                                    binding::kNoListenerMax, false, &tracker);
 
   v8::Local<v8::Function> listener = FunctionFromString(context, kFunction);

extensions/renderer/bindings/event_emitter_unittest.cc

@@ -18,6 +18,15 @@
 
 namespace extensions {
 
+namespace {
+
+APIEventListeners::ContextOwnerIdGetter CreateContextOwnerIdGetter() {
+  return base::BindRepeating(
+      [](v8::Local<v8::Context>) { return std::string("context"); });
+}
+
+}  // namespace
+
 class EventEmitterUnittest : public APIBindingTest {
  public:
   EventEmitterUnittest() = default;
@@ -41,8 +50,8 @@ TEST_F(EventEmitterUnittest, TestDispatchMethod) {
 
   ListenerTracker tracker;
   auto listeners = std::make_unique<UnfilteredEventListeners>(
-      base::DoNothing(), "event", "context", binding::kNoListenerMax, true,
-      &tracker);
+      base::DoNothing(), "event", CreateContextOwnerIdGetter(),
+      binding::kNoListenerMax, true, &tracker);
 
   auto log_error = [](std::vector<std::string>* errors,
                       v8::Local<v8::Context> context,
@@ -144,8 +153,8 @@ TEST_F(EventEmitterUnittest, ListenersDestroyingContext) {
 
   ListenerTracker tracker;
   auto listeners = std::make_unique<UnfilteredEventListeners>(
-      base::DoNothing(), "event", "context", binding::kNoListenerMax, true,
-      &tracker);
+      base::DoNothing(), "event", CreateContextOwnerIdGetter(),
+      binding::kNoListenerMax, true, &tracker);
   ExceptionHandler exception_handler(base::BindRepeating(
       [](v8::Local<v8::Context> context, const std::string& error) {}));
   gin::Handle<EventEmitter> event = gin::CreateHandle(