From 9bcb5f9146db42bc38b6bb744fb0cf518a0205be Mon Sep 17 00:00:00 2001
From: vladlosev <vladlosev@8415998a-534a-0410-bf83-d39667b30386>
Date: Mon, 24 Oct 2011 23:41:07 +0000
Subject: [PATCH] Fixes a lock reentrancy when destroying a mock causes
destruction of another mock (issue 79) (by Aaron Jacobs).
---
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 1953e43a..36c47d67 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<const OnCallSpec<F>*>(*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 06299784..6599325d 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 29d47d12..9177b322 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<MockA>*));
+
+ 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<MockA> 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<MockA> 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