From 018ebceeaffa48eb3aa5343f5872dd3eba6b97cb Mon Sep 17 00:00:00 2001 From: Ken Rockot Date: Fri, 1 Sep 2017 02:52:10 +0000 Subject: [PATCH] Mojo: Fix nested SyncHandleWatcher Wait We defer event callback list modification until the outermost Wait() unwinds, but we need to ensure that the internal WaitSet has events removed immediately within UnregisterEvent if the event has no more valid callbacks registered. This allows callers to make the assumption that exclusively registered events are not referenced beyond a call to UnregisterEvent, which is an assumption made by production code. Adds tests for SyncHandleRegistry event watching too. BUG=761097 Change-Id: Ibbf6eee335879bba6b732637944338f046bf37cb Reviewed-on: https://chromium-review.googlesource.com/646491 Reviewed-by: Yuzhu Shen Commit-Queue: Ken Rockot Cr-Commit-Position: refs/heads/master@{#499116} --- .../cpp/bindings/lib/sync_handle_registry.cc | 39 +-- mojo/public/cpp/bindings/tests/BUILD.gn | 1 + .../tests/sync_handle_registry_unittest.cc | 257 ++++++++++++++++++ 3 files changed, 279 insertions(+), 18 deletions(-) create mode 100644 mojo/public/cpp/bindings/tests/sync_handle_registry_unittest.cc 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