Fixes a lock reentrancy when destroying a mock causes destruction of another...

Fixes a lock reentrancy when destroying a mock causes destruction of another mock (issue 79) (by Aaron Jacobs).

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:

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;
 }
 

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