From 6260125d98ddc9bea2cfef739b0f7378f0264a3e Mon Sep 17 00:00:00 2001 From: vladlosev Date: Mon, 24 Oct 2011 23:41:07 +0000 Subject: Fixes a lock reentrancy when destroying a mock causes destruction of another mock (issue 79) (by Aaron Jacobs). git-svn-id: http://googlemock.googlecode.com/svn/trunk@403 8415998a-534a-0410-bf83-d39667b30386 --- include/gmock/gmock-spec-builders.h | 21 ++++++++++++--- src/gmock-spec-builders.cc | 16 ++++++++++- test/gmock-spec-builders_test.cc | 53 ++++++++++++++++++++++++++++++++++++- 3 files changed, 85 insertions(+), 5 deletions(-) diff --git a/include/gmock/gmock-spec-builders.h b/include/gmock/gmock-spec-builders.h index 1953e43..36c47d6 100644 --- a/include/gmock/gmock-spec-builders.h +++ b/include/gmock/gmock-spec-builders.h @@ -1475,12 +1475,27 @@ class FunctionMockerBase : public UntypedFunctionMockerBase { virtual void ClearDefaultActionsLocked() GTEST_EXCLUSIVE_LOCK_REQUIRED_(g_gmock_mutex) { g_gmock_mutex.AssertHeld(); + + // Deleting our default actions may trigger other mock objects to be + // deleted, for example if an action contains a reference counted smart + // pointer to that mock object, and that is the last reference. So if we + // delete our actions within the context of the global mutex we may deadlock + // when this method is called again. Instead, make a copy of the set of + // actions to delete, clear our set within the mutex, and then delete the + // actions outside of the mutex. + UntypedOnCallSpecs specs_to_delete; + untyped_on_call_specs_.swap(specs_to_delete); + + g_gmock_mutex.Unlock(); for (UntypedOnCallSpecs::const_iterator it = - untyped_on_call_specs_.begin(); - it != untyped_on_call_specs_.end(); ++it) { + specs_to_delete.begin(); + it != specs_to_delete.end(); ++it) { delete static_cast*>(*it); } - untyped_on_call_specs_.clear(); + + // Lock the mutex again, since the caller expects it to be locked when we + // return. + g_gmock_mutex.Lock(); } protected: diff --git a/src/gmock-spec-builders.cc b/src/gmock-spec-builders.cc index 0629978..6599325 100644 --- a/src/gmock-spec-builders.cc +++ b/src/gmock-spec-builders.cc @@ -480,7 +480,21 @@ bool UntypedFunctionMockerBase::VerifyAndClearExpectationsLocked() untyped_expectation->line(), ss.str()); } } - untyped_expectations_.clear(); + + // Deleting our expectations may trigger other mock objects to be deleted, for + // example if an action contains a reference counted smart pointer to that + // mock object, and that is the last reference. So if we delete our + // expectations within the context of the global mutex we may deadlock when + // this method is called again. Instead, make a copy of the set of + // expectations to delete, clear our set within the mutex, and then clear the + // copied set outside of it. + UntypedExpectations expectations_to_delete; + untyped_expectations_.swap(expectations_to_delete); + + g_gmock_mutex.Unlock(); + expectations_to_delete.clear(); + g_gmock_mutex.Lock(); + return expectations_met; } diff --git a/test/gmock-spec-builders_test.cc b/test/gmock-spec-builders_test.cc index 29d47d1..9177b32 100644 --- a/test/gmock-spec-builders_test.cc +++ b/test/gmock-spec-builders_test.cc @@ -88,13 +88,14 @@ using testing::Mock; using testing::Ne; using testing::Return; using testing::Sequence; +using testing::SetArgPointee; using testing::internal::ExpectationTester; using testing::internal::FormatFileLocation; -using testing::internal::g_gmock_mutex; using testing::internal::kErrorVerbosity; using testing::internal::kInfoVerbosity; using testing::internal::kWarningVerbosity; using testing::internal::String; +using testing::internal::linked_ptr; using testing::internal::string; #if GTEST_HAS_STREAM_REDIRECTION @@ -157,6 +158,16 @@ class MockB { GTEST_DISALLOW_COPY_AND_ASSIGN_(MockB); }; +class ReferenceHoldingMock { + public: + ReferenceHoldingMock() {} + + MOCK_METHOD1(AcceptReference, void(linked_ptr*)); + + private: + GTEST_DISALLOW_COPY_AND_ASSIGN_(ReferenceHoldingMock); +}; + // Tests that EXPECT_CALL and ON_CALL compile in a presence of macro // redefining a mock method name. This could happen, for example, when // the tested code #includes Win32 API headers which define many APIs @@ -2439,6 +2450,46 @@ TEST(VerifyAndClearTest, DoesNotAffectOtherMockObjects) { EXPECT_EQ(2, b1.DoB(0)); } +TEST(VerifyAndClearTest, + DestroyingChainedMocksDoesNotDeadlockThroughExpectations) { + linked_ptr a(new MockA); + ReferenceHoldingMock test_mock; + + // EXPECT_CALL stores a reference to a inside test_mock. + EXPECT_CALL(test_mock, AcceptReference(_)) + .WillRepeatedly(SetArgPointee<0>(a)); + + // Throw away the reference to the mock that we have in a. After this, the + // only reference to it is stored by test_mock. + a.reset(); + + // When test_mock goes out of scope, it destroys the last remaining reference + // to the mock object originally pointed to by a. This will cause the MockA + // destructor to be called from inside the ReferenceHoldingMock destructor. + // The state of all mocks is protected by a single global lock, but there + // should be no deadlock. +} + +TEST(VerifyAndClearTest, + DestroyingChainedMocksDoesNotDeadlockThroughDefaultAction) { + linked_ptr a(new MockA); + ReferenceHoldingMock test_mock; + + // ON_CALL stores a reference to a inside test_mock. + ON_CALL(test_mock, AcceptReference(_)) + .WillByDefault(SetArgPointee<0>(a)); + + // Throw away the reference to the mock that we have in a. After this, the + // only reference to it is stored by test_mock. + a.reset(); + + // When test_mock goes out of scope, it destroys the last remaining reference + // to the mock object originally pointed to by a. This will cause the MockA + // destructor to be called from inside the ReferenceHoldingMock destructor. + // The state of all mocks is protected by a single global lock, but there + // should be no deadlock. +} + // Tests that a mock function's action can call a mock function // (either the same function or a different one) either as an explicit // action or as a default action without causing a dead lock. It -- cgit v1.2.1