0

[Extensions Bindings] Notify of event unregistration on invalidation

When contexts are invalidated, we consider the events in that context
unregistered and notify the browser accordingly. Hook up an
InvalidateContext() method on APIEventHandler and call it through
blink's WillReleaseScriptContext().

Add associated tests.

BUG=653596

Review-Url: https://codereview.chromium.org/2722463006
Cr-Commit-Position: refs/heads/master@{#454911}
This commit is contained in:
rdevlin.cronin
2017-03-06 11:18:00 -08:00
committed by Commit bot
parent 56a39345f2
commit 317a9b943c
10 changed files with 148 additions and 16 deletions

@ -124,6 +124,10 @@ void APIBindingsSystem::RegisterCustomType(const std::string& type_name,
custom_types_[type_name] = function;
}
void APIBindingsSystem::WillReleaseContext(v8::Local<v8::Context> context) {
event_handler_.InvalidateContext(context);
}
v8::Local<v8::Object> APIBindingsSystem::CreateCustomType(
v8::Local<v8::Context> context,
const std::string& type_name,

@ -84,6 +84,9 @@ class APIBindingsSystem {
void RegisterCustomType(const std::string& type_name,
const CustomTypeHandler& function);
// Handles any cleanup necessary before releasing the given |context|.
void WillReleaseContext(v8::Local<v8::Context> context);
APIRequestHandler* request_handler() { return &request_handler_; }
APIEventHandler* event_handler() { return &event_handler_; }
APITypeReferenceMap* type_reference_map() { return &type_reference_map_; }

@ -122,6 +122,10 @@ class APIBindingsSystemTestBase : public APIBindingTest {
}
void TearDown() override {
{
v8::HandleScope handle_scope(isolate());
bindings_system_->WillReleaseContext(ContextLocal());
}
bindings_system_.reset();
APIBindingTest::TearDown();
}

@ -28,18 +28,10 @@ const char kExtensionAPIEventPerContextKey[] = "extension_api_events";
struct APIEventPerContextData : public base::SupportsUserData::Data {
APIEventPerContextData(v8::Isolate* isolate) : isolate(isolate) {}
~APIEventPerContextData() override {
v8::HandleScope scope(isolate);
// We explicitly clear the event data map here to remove all references to
// v8 objects. This helps us avoid cycles in v8 where an event listener
// could hold a reference to the event, which in turn holds the reference
// to the listener.
for (const auto& pair : emitters) {
EventEmitter* emitter = nullptr;
gin::Converter<EventEmitter*>::FromV8(
isolate, pair.second.Get(isolate), &emitter);
CHECK(emitter);
emitter->listeners()->clear();
}
DCHECK(emitters.empty())
<< "|emitters| should have been cleared by InvalidateContext()";
DCHECK(massagers.empty())
<< "|massagers| should have been cleared by InvalidateContext()";
}
// The associated v8::Isolate. Since this object is cleaned up at context
@ -198,6 +190,39 @@ void APIEventHandler::RegisterArgumentMassager(
data->massagers[event_name].Reset(context->GetIsolate(), massager);
}
void APIEventHandler::InvalidateContext(v8::Local<v8::Context> context) {
gin::PerContextData* per_context_data = gin::PerContextData::From(context);
DCHECK(per_context_data);
APIEventPerContextData* data = static_cast<APIEventPerContextData*>(
per_context_data->GetUserData(kExtensionAPIEventPerContextKey));
if (!data)
return;
v8::Isolate* isolate = context->GetIsolate();
v8::HandleScope scope(isolate);
// This loop *shouldn't* allow any self-modification (i.e., no listeners
// should be added or removed as a result of the iteration). If that changes,
// we'll need to cache the listeners elsewhere before iterating.
for (const auto& pair : data->emitters) {
EventEmitter* emitter = nullptr;
gin::Converter<EventEmitter*>::FromV8(isolate, pair.second.Get(isolate),
&emitter);
CHECK(emitter);
emitter->Invalidate();
// When the context is shut down, all listeners are removed.
listeners_changed_.Run(
pair.first, binding::EventListenersChanged::NO_LISTENERS, context);
}
data->emitters.clear();
data->massagers.clear();
// InvalidateContext() is called shortly (and, theoretically, synchronously)
// before the PerContextData is deleted. We have a check that guarantees that
// no new EventEmitters are created after the PerContextData is deleted, so
// no new emitters should be created after this point.
}
size_t APIEventHandler::GetNumEventListenersForTesting(
const std::string& event_name,
v8::Local<v8::Context> context) {

@ -57,6 +57,12 @@ class APIEventHandler {
size_t GetNumEventListenersForTesting(const std::string& event_name,
v8::Local<v8::Context> context);
// Invalidates listeners for the given |context|. It's a shame we have to
// have this separately (as opposed to hooking into e.g. a PerContextData
// destructor), but we need to do this before the context is fully removed
// (because the associated extension ScriptContext needs to be valid).
void InvalidateContext(v8::Local<v8::Context> context);
private:
// Method to run a given v8::Function. Curried in for testing.
binding::RunJSFunction call_js_;

@ -129,6 +129,8 @@ TEST_F(APIEventHandlerTest, AddingRemovingAndQueryingEventListeners) {
gin::Converter<bool>::FromV8(isolate(), result, &has_listeners));
EXPECT_FALSE(has_listeners);
}
handler.InvalidateContext(context);
}
// Tests listening for and firing different events.
@ -227,6 +229,8 @@ TEST_F(APIEventHandlerTest, FiringEvents) {
EXPECT_EQ(2, get_fired_count("alphaCount1"));
EXPECT_EQ(2, get_fired_count("alphaCount2"));
EXPECT_EQ(1, get_fired_count("betaCount"));
handler.InvalidateContext(context);
}
// Tests firing events with arguments.
@ -264,6 +268,8 @@ TEST_F(APIEventHandlerTest, EventArguments) {
EXPECT_EQ(
ReplaceSingleQuotes(kArguments),
GetStringPropertyFromObject(context->Global(), context, "eventArgs"));
handler.InvalidateContext(context);
}
// Test dispatching events to multiple contexts.
@ -349,6 +355,9 @@ TEST_F(APIEventHandlerTest, MultipleContexts) {
GetStringPropertyFromObject(context_b->Global(), context_b,
"eventArgs"));
}
handler.InvalidateContext(context_a);
handler.InvalidateContext(context_b);
}
TEST_F(APIEventHandlerTest, DifferentCallingMethods) {
@ -404,6 +413,8 @@ TEST_F(APIEventHandlerTest, DifferentCallingMethods) {
context, 1, args);
}
EXPECT_EQ(2u, handler.GetNumEventListenersForTesting(kEventName, context));
handler.InvalidateContext(context);
}
TEST_F(APIEventHandlerTest, TestDispatchFromJs) {
@ -444,6 +455,8 @@ TEST_F(APIEventHandlerTest, TestDispatchFromJs) {
EXPECT_EQ("[42,\"foo\",{\"bar\":\"baz\"}]",
GetStringPropertyFromObject(
context->Global(), context, "eventArgs"));
handler.InvalidateContext(context);
}
// Test listeners that remove themselves in their handling of the event.
@ -500,6 +513,7 @@ TEST_F(APIEventHandlerTest, RemovingListenersWhileHandlingEvent) {
handler.FireEventInContext(kEventName, context, base::ListValue());
EXPECT_EQ(0u, handler.GetNumEventListenersForTesting(kEventName, context));
handler.InvalidateContext(context);
// TODO(devlin): Another possible test: register listener a and listener b,
// where a removes b and b removes a. Theoretically, only one should be
// notified. Investigate what we currently do in JS-style bindings.
@ -584,6 +598,8 @@ TEST_F(APIEventHandlerTest, TestEventListenersThrowingExceptions) {
EXPECT_EQ("[42]", GetStringPropertyFromObject(context->Global(), context,
"eventArgs"));
EXPECT_TRUE(did_throw);
handler.InvalidateContext(context);
}
// Tests being notified as listeners are added or removed from events.
@ -706,6 +722,26 @@ TEST_F(APIEventHandlerTest, CallbackNotifications) {
}
EXPECT_EQ(1u,
handler.GetNumEventListenersForTesting(kEventName1, context_b));
// When the contexts are invalidated, we should receive listener removed
// notifications.
EXPECT_CALL(
change_handler,
Run(kEventName1, binding::EventListenersChanged::NO_LISTENERS, context_a))
.Times(1);
EXPECT_CALL(
change_handler,
Run(kEventName2, binding::EventListenersChanged::NO_LISTENERS, context_a))
.Times(1);
handler.InvalidateContext(context_a);
::testing::Mock::VerifyAndClearExpectations(&change_handler);
EXPECT_CALL(
change_handler,
Run(kEventName1, binding::EventListenersChanged::NO_LISTENERS, context_b))
.Times(1);
handler.InvalidateContext(context_b);
::testing::Mock::VerifyAndClearExpectations(&change_handler);
}
// Test registering an argument massager for a given event.
@ -755,6 +791,8 @@ TEST_F(APIEventHandlerTest, TestArgumentMassagers) {
EXPECT_EQ(
"[\"primary\",\"secondary\"]",
GetStringPropertyFromObject(context->Global(), context, "eventArgs"));
handler.InvalidateContext(context);
}
// Test registering an argument massager for a given event and dispatching
@ -821,6 +859,8 @@ TEST_F(APIEventHandlerTest, TestArgumentMassagersAsyncDispatch) {
EXPECT_EQ(
"[\"primary\",\"secondary\"]",
GetStringPropertyFromObject(context->Global(), context, "eventArgs"));
handler.InvalidateContext(context);
}
// Test registering an argument massager and never dispatching.
@ -858,6 +898,8 @@ TEST_F(APIEventHandlerTest, TestArgumentMassagersNeverDispatch) {
// Nothing should blow up. (We tested in the previous test that the event
// isn't notified without calling dispatch, so all there is to test here is
// that we don't crash.)
handler.InvalidateContext(context);
}
} // namespace extensions

@ -52,7 +52,19 @@ void EventEmitter::Fire(v8::Local<v8::Context> context,
}
}
void EventEmitter::Invalidate() {
valid_ = false;
listeners_.clear();
}
void EventEmitter::AddListener(gin::Arguments* arguments) {
// If script from another context maintains a reference to this object, it's
// possible that functions can be called after this object's owning context
// is torn down and released by blink. We don't support this behavior, but
// we need to make sure nothing crashes, so early out of methods.
if (!valid_)
return;
v8::Local<v8::Function> listener;
// TODO(devlin): For some reason, we don't throw an error when someone calls
// add/removeListener with no argument. We probably should. For now, keep
@ -78,6 +90,10 @@ void EventEmitter::AddListener(gin::Arguments* arguments) {
}
void EventEmitter::RemoveListener(gin::Arguments* arguments) {
// See comment in AddListener().
if (!valid_)
return;
v8::Local<v8::Function> listener;
// See comment in AddListener().
if (!arguments->GetNext(&listener))
@ -107,6 +123,9 @@ bool EventEmitter::HasListeners() {
}
void EventEmitter::Dispatch(gin::Arguments* arguments) {
if (!valid_)
return;
if (listeners_.empty())
return;
v8::HandleScope handle_scope(arguments->isolate());

@ -40,7 +40,11 @@ class EventEmitter final : public gin::Wrappable<EventEmitter> {
void Fire(v8::Local<v8::Context> context,
std::vector<v8::Local<v8::Value>>* args);
Listeners* listeners() { return &listeners_; }
// Removes all listeners and marks this object as invalid so that no more
// are added.
void Invalidate();
const Listeners* listeners() const { return &listeners_; }
private:
// Bound methods for the Event JS object.
@ -50,6 +54,10 @@ class EventEmitter final : public gin::Wrappable<EventEmitter> {
bool HasListeners();
void Dispatch(gin::Arguments* arguments);
// Whether or not this object is still valid; false upon context release.
// When invalid, no listeners can be added or removed.
bool valid_ = true;
Listeners listeners_;
binding::RunJSFunction run_js_;

@ -368,7 +368,10 @@ void NativeExtensionBindingsSystem::DidCreateScriptContext(
}
void NativeExtensionBindingsSystem::WillReleaseScriptContext(
ScriptContext* context) {}
ScriptContext* context) {
v8::HandleScope handle_scope(context->isolate());
api_system_.WillReleaseContext(context->v8_context());
}
void NativeExtensionBindingsSystem::UpdateBindingsForContext(
ScriptContext* context) {

@ -99,8 +99,12 @@ class NativeExtensionBindingsSystemUnittest : public APIBindingTest {
}
void TearDown() override {
for (auto* context : raw_script_contexts_)
event_change_handler_.reset();
for (auto* context : raw_script_contexts_) {
bindings_system_->WillReleaseScriptContext(context);
script_context_set_->Remove(context);
}
base::RunLoop().RunUntilIdle();
script_context_set_.reset();
bindings_system_.reset();
@ -141,6 +145,20 @@ class NativeExtensionBindingsSystemUnittest : public APIBindingTest {
return raw_script_context;
}
void DisposeMainScriptContext() {
v8::Local<v8::Context> context = ContextLocal();
auto iter =
std::find_if(raw_script_contexts_.begin(), raw_script_contexts_.end(),
[context](ScriptContext* script_context) {
return script_context->v8_context() == context;
});
ASSERT_TRUE(iter != raw_script_contexts_.end());
bindings_system_->WillReleaseScriptContext(*iter);
DisposeContext();
script_context_set_->Remove(*iter);
raw_script_contexts_.erase(iter);
}
void RegisterExtension(const ExtensionId& id) { extension_ids_.insert(id); }
void InitEventChangeHandler() {
@ -361,7 +379,7 @@ TEST_F(NativeExtensionBindingsSystemUnittest,
ASSERT_FALSE(first_idle_object.IsEmpty());
EXPECT_TRUE(first_idle_object->IsObject());
DisposeContext();
DisposeMainScriptContext();
// Check an API that was instantiated....
v8::Local<v8::Value> second_idle_object =