diff --git a/mojo/public/cpp/bindings/lib/sync_handle_registry.cc b/mojo/public/cpp/bindings/lib/sync_handle_registry.cc index a7a5a6f92e8870..6816de0ec5c242 100644 --- a/mojo/public/cpp/bindings/lib/sync_handle_registry.cc +++ b/mojo/public/cpp/bindings/lib/sync_handle_registry.cc @@ -72,15 +72,12 @@ void SyncHandleRegistry::RegisterEvent(base::WaitableEvent* event, it = result.first; } - auto& callbacks = it->second.container(); - if (callbacks.empty()) { - // AddEvent() must succeed since we only attempt it when there are - // previously no callbacks registered for this event. - MojoResult rv = wait_set_.AddEvent(event); - DCHECK_EQ(MOJO_RESULT_OK, rv); - } + // The event may already be in the WaitSet, but we don't care. This will be a + // no-op in that case, which is more efficient than scanning the list of + // callbacks to see if any are valid. + wait_set_.AddEvent(event); - callbacks.push_back(callback); + it->second.container().push_back(callback); } void SyncHandleRegistry::UnregisterEvent(base::WaitableEvent* event, @@ -89,6 +86,7 @@ void SyncHandleRegistry::UnregisterEvent(base::WaitableEvent* event, if (it == events_.end()) return; + bool has_valid_callbacks = false; auto& callbacks = it->second.container(); if (is_dispatching_event_callbacks_) { // Not safe to remove any elements from |callbacks| here since an outer @@ -96,6 +94,8 @@ void SyncHandleRegistry::UnregisterEvent(base::WaitableEvent* event, for (auto& cb : callbacks) { if (cb.Equals(callback)) cb.Reset(); + else if (cb) + has_valid_callbacks = true; } remove_invalid_event_callbacks_after_dispatch_ = true; } else { @@ -104,11 +104,18 @@ void SyncHandleRegistry::UnregisterEvent(base::WaitableEvent* event, return cb.Equals(callback); }), callbacks.end()); - if (callbacks.empty()) { + if (callbacks.empty()) events_.erase(it); - MojoResult rv = wait_set_.RemoveEvent(event); - DCHECK_EQ(MOJO_RESULT_OK, rv); - } + else + has_valid_callbacks = true; + } + + if (!has_valid_callbacks) { + // Regardless of whether or not we're nested within a Wait(), we need to + // ensure that |event| is removed from the WaitSet before returning if this + // was the last callback registered for it. + MojoResult rv = wait_set_.RemoveEvent(event); + DCHECK_EQ(MOJO_RESULT_OK, rv); } } @@ -180,14 +187,10 @@ void SyncHandleRegistry::RemoveInvalidEventCallbacks() { std::remove_if(callbacks.begin(), callbacks.end(), [](const base::Closure& callback) { return !callback; }), callbacks.end()); - if (callbacks.empty()) { - MojoResult rv = wait_set_.RemoveEvent(it->first); - DCHECK_EQ(MOJO_RESULT_OK, rv); - + if (callbacks.empty()) events_.erase(it++); - } else { + else ++it; - } } } diff --git a/mojo/public/cpp/bindings/tests/BUILD.gn b/mojo/public/cpp/bindings/tests/BUILD.gn index a7cd611fcd3800..456da1300e3cb9 100644 --- a/mojo/public/cpp/bindings/tests/BUILD.gn +++ b/mojo/public/cpp/bindings/tests/BUILD.gn @@ -35,6 +35,7 @@ source_set("tests") { "sample_service_unittest.cc", "serialization_warning_unittest.cc", "struct_unittest.cc", + "sync_handle_registry_unittest.cc", "sync_method_unittest.cc", "type_conversion_unittest.cc", "union_unittest.cc", diff --git a/mojo/public/cpp/bindings/tests/sync_handle_registry_unittest.cc b/mojo/public/cpp/bindings/tests/sync_handle_registry_unittest.cc new file mode 100644 index 00000000000000..c0e985b0233a07 --- /dev/null +++ b/mojo/public/cpp/bindings/tests/sync_handle_registry_unittest.cc @@ -0,0 +1,257 @@ +// 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. + +#include "mojo/public/cpp/bindings/sync_handle_registry.h" +#include "base/bind.h" +#include "base/memory/ptr_util.h" +#include "base/memory/ref_counted.h" +#include "base/synchronization/waitable_event.h" +#include "testing/gtest/include/gtest/gtest.h" + +namespace mojo { + +class SyncHandleRegistryTest : public testing::Test { + public: + SyncHandleRegistryTest() : registry_(SyncHandleRegistry::current()) {} + + const scoped_refptr& registry() { return registry_; } + + private: + scoped_refptr registry_; + + DISALLOW_COPY_AND_ASSIGN(SyncHandleRegistryTest); +}; + +TEST_F(SyncHandleRegistryTest, DuplicateEventRegistration) { + bool called1 = false; + bool called2 = false; + auto callback = [](bool* called) { *called = true; }; + auto callback1 = base::Bind(callback, &called1); + auto callback2 = base::Bind(callback, &called2); + + base::WaitableEvent e(base::WaitableEvent::ResetPolicy::MANUAL, + base::WaitableEvent::InitialState::SIGNALED); + registry()->RegisterEvent(&e, callback1); + registry()->RegisterEvent(&e, callback2); + + const bool* stop_flags[] = {&called1, &called2}; + registry()->Wait(stop_flags, 2); + + EXPECT_TRUE(called1); + EXPECT_TRUE(called2); + registry()->UnregisterEvent(&e, callback1); + + called1 = false; + called2 = false; + + registry()->Wait(stop_flags, 2); + + EXPECT_FALSE(called1); + EXPECT_TRUE(called2); + + registry()->UnregisterEvent(&e, callback2); +} + +TEST_F(SyncHandleRegistryTest, UnregisterDuplicateEventInNestedWait) { + base::WaitableEvent e(base::WaitableEvent::ResetPolicy::MANUAL, + base::WaitableEvent::InitialState::SIGNALED); + bool called1 = false; + bool called2 = false; + bool called3 = false; + auto callback1 = base::Bind([](bool* called) { *called = true; }, &called1); + auto callback2 = base::Bind( + [](base::WaitableEvent* e, const base::Closure& other_callback, + scoped_refptr registry, bool* called) { + registry->UnregisterEvent(e, other_callback); + *called = true; + }, + &e, callback1, registry(), &called2); + auto callback3 = base::Bind([](bool* called) { *called = true; }, &called3); + + registry()->RegisterEvent(&e, callback1); + registry()->RegisterEvent(&e, callback2); + registry()->RegisterEvent(&e, callback3); + + const bool* stop_flags[] = {&called1, &called2, &called3}; + registry()->Wait(stop_flags, 3); + + // We don't make any assumptions about the order in which callbacks run, so + // we can't check |called1| - it may or may not get set depending on internal + // details. All we know is |called2| should be set, and a subsequent wait + // should definitely NOT set |called1|. + EXPECT_TRUE(called2); + EXPECT_TRUE(called3); + + called1 = false; + called2 = false; + called3 = false; + + registry()->UnregisterEvent(&e, callback2); + registry()->Wait(stop_flags, 3); + + EXPECT_FALSE(called1); + EXPECT_FALSE(called2); + EXPECT_TRUE(called3); +} + +TEST_F(SyncHandleRegistryTest, UnregisterAndRegisterForNewEventInCallback) { + auto e = base::MakeUnique( + base::WaitableEvent::ResetPolicy::MANUAL, + base::WaitableEvent::InitialState::SIGNALED); + bool called = false; + base::Closure callback_holder; + auto callback = base::Bind( + [](std::unique_ptr* e, + base::Closure* callback_holder, + scoped_refptr registry, bool* called) { + EXPECT_FALSE(*called); + + registry->UnregisterEvent(e->get(), *callback_holder); + e->reset(); + *called = true; + + base::WaitableEvent nested_event( + base::WaitableEvent::ResetPolicy::MANUAL, + base::WaitableEvent::InitialState::SIGNALED); + bool nested_called = false; + auto nested_callback = + base::Bind([](bool* called) { *called = true; }, &nested_called); + registry->RegisterEvent(&nested_event, nested_callback); + const bool* stop_flag = &nested_called; + registry->Wait(&stop_flag, 1); + registry->UnregisterEvent(&nested_event, nested_callback); + }, + &e, &callback_holder, registry(), &called); + callback_holder = callback; + + registry()->RegisterEvent(e.get(), callback); + + const bool* stop_flag = &called; + registry()->Wait(&stop_flag, 1); + EXPECT_TRUE(called); +} + +TEST_F(SyncHandleRegistryTest, UnregisterAndRegisterForSameEventInCallback) { + base::WaitableEvent e(base::WaitableEvent::ResetPolicy::MANUAL, + base::WaitableEvent::InitialState::SIGNALED); + bool called = false; + base::Closure callback_holder; + auto callback = base::Bind( + [](base::WaitableEvent* e, base::Closure* callback_holder, + scoped_refptr registry, bool* called) { + EXPECT_FALSE(*called); + + registry->UnregisterEvent(e, *callback_holder); + *called = true; + + bool nested_called = false; + auto nested_callback = + base::Bind([](bool* called) { *called = true; }, &nested_called); + registry->RegisterEvent(e, nested_callback); + const bool* stop_flag = &nested_called; + registry->Wait(&stop_flag, 1); + registry->UnregisterEvent(e, nested_callback); + + EXPECT_TRUE(nested_called); + }, + &e, &callback_holder, registry(), &called); + callback_holder = callback; + + registry()->RegisterEvent(&e, callback); + + const bool* stop_flag = &called; + registry()->Wait(&stop_flag, 1); + EXPECT_TRUE(called); +} + +TEST_F(SyncHandleRegistryTest, RegisterDuplicateEventFromWithinCallback) { + base::WaitableEvent e(base::WaitableEvent::ResetPolicy::MANUAL, + base::WaitableEvent::InitialState::SIGNALED); + bool called = false; + int call_count = 0; + auto callback = base::Bind( + [](base::WaitableEvent* e, scoped_refptr registry, + bool* called, int* call_count) { + // Don't re-enter. + ++(*call_count); + if (*called) + return; + + *called = true; + + bool called2 = false; + auto callback2 = + base::Bind([](bool* called) { *called = true; }, &called2); + registry->RegisterEvent(e, callback2); + + const bool* stop_flag = &called2; + registry->Wait(&stop_flag, 1); + + registry->UnregisterEvent(e, callback2); + }, + &e, registry(), &called, &call_count); + + registry()->RegisterEvent(&e, callback); + + const bool* stop_flag = &called; + registry()->Wait(&stop_flag, 1); + + EXPECT_TRUE(called); + EXPECT_EQ(2, call_count); + + registry()->UnregisterEvent(&e, callback); +} + +TEST_F(SyncHandleRegistryTest, UnregisterUniqueEventInNestedWait) { + auto e1 = base::MakeUnique( + base::WaitableEvent::ResetPolicy::MANUAL, + base::WaitableEvent::InitialState::NOT_SIGNALED); + base::WaitableEvent e2(base::WaitableEvent::ResetPolicy::MANUAL, + base::WaitableEvent::InitialState::SIGNALED); + bool called1 = false; + bool called2 = false; + auto callback1 = base::Bind([](bool* called) { *called = true; }, &called1); + auto callback2 = base::Bind( + [](std::unique_ptr* e1, + const base::Closure& other_callback, + scoped_refptr registry, bool* called) { + // Prevent re-entrancy. + if (*called) + return; + + registry->UnregisterEvent(e1->get(), other_callback); + *called = true; + e1->reset(); + + // Nest another wait. + bool called3 = false; + auto callback3 = + base::Bind([](bool* called) { *called = true; }, &called3); + base::WaitableEvent e3(base::WaitableEvent::ResetPolicy::MANUAL, + base::WaitableEvent::InitialState::SIGNALED); + registry->RegisterEvent(&e3, callback3); + + // This nested Wait() must not attempt to wait on |e1| since it has + // been unregistered. This would crash otherwise, since |e1| has been + // deleted. See http://crbug.com/761097. + const bool* stop_flags[] = {&called3}; + registry->Wait(stop_flags, 1); + + EXPECT_TRUE(called3); + registry->UnregisterEvent(&e3, callback3); + }, + &e1, callback1, registry(), &called2); + + registry()->RegisterEvent(e1.get(), callback1); + registry()->RegisterEvent(&e2, callback2); + + const bool* stop_flags[] = {&called1, &called2}; + registry()->Wait(stop_flags, 2); + + EXPECT_TRUE(called2); + + registry()->UnregisterEvent(&e2, callback2); +} + +} // namespace mojo