Crash on Windows when leaked mock exists

[basejumpa]

Describe the bug

When you instantiate a mock object globally instead within a test function this produces a "leaked mock" in terms of gmock.

This is not clean because the mock object need to terminate at the end of each test function to check the actual calls of the code inside the test function against the expectations. When you do it as intended then you place the mock object variable inside the test function. Then the mock object will be deleted automatically on test function return and the destructor is called to do the checks.

A leaked mock leads ...

• on Linux this leads to a message on the console but the test executable exits gracefully. It also gives warnings on the console about leaked object(s);
• on Windows leads to a crash due to a segmentation fault of the test executable.

Expected: gmock/gtest shall handle leaked mock objects on windows as it does on Linux.

Steps to reproduce the bug

I created a repository with github-actions which reproduces the issue: https://github.com/basejumpa/gmock_segfault

The build output on Windows see https://github.com/basejumpa/gmock_segfault/runs/7897083476?check_suite_focus=true#step:3:113

The build output on Linux see https://github.com/basejumpa/gmock_segfault/runs/7897083469?check_suite_focus=true#step:4:78

Does the bug persist in the most recent commit?

The problem occurs with googletest release-1.12.1

and also with HEAD of branch main as of this writing (commit c0e032e )

What operating system and version are you using?

Windows . See https://github.com/actions/runner-images/blob/win22/20220816.1/images/win/Windows2022-Readme.md

What compiler and version are you using?

Visual Studio Enterprise 2022.

But it also occurs with clang 14.x

What build system are you using?

CMake 3.24.0

Additional context

[higher-performance]

I managed to reproduce this. It turns out this happens during the build phase in Visual Studio, not during normal execution, because the build process itself calls the compiled executable as a post-build event. To reproduce the crash, we need to run: test_expectation_not_met__leaked.exe --gtest_list_tests.

It turns out the issue is that Mockup in your .cc file uses FunctionMocker, which inherits from UntypedFunctionMockerBase, which uses g_mock_object_registry on construction, which is a static variable initialized in global scope in gmock-spec-builders.cc, which means its construction and destruction order are not actually guaranteed to be correct with respect to the Mockup variable. In particular, on destruction, Mock::UnregisterLocked(this) uses g_mock_object_registry, which is already destructed by the time it is called. I am guessing the reason this "works" on Linux is that MSVC in debug mode performs extra checks on destruction, whereas Linux lets it slide. It certainly does not appear to be well-defined on either platform.

[higher-performance]

Before we attempt to add a workaround for this, could you please clarify what the use case for this is? Normally I don't believe GoogleTest supports global mock objects, and I'm afraid I don't understand why they are needed in your case. If you could please explain/illustrate what the motivation for this is and why we should support it then we may consider seeing if a workaround is warranted.

Please do also note that the current behavior on Linux is broken just as it is on Windows; it's just that there is no explicit crash due to the lack of extra checks being done on destruction. So if the Linux behavior is something whose correctness you rely on, please explain how and what the use case is. Otherwise, we may close this issue as not-supported. Thanks!

[higher-performance]

I'm closing this issue now since we haven't heard back regarding a motivation for this. However, if you do find a need for this, feel free to reopen and explain the use case. (Otherwise, we prefer to avoid adding support for this without a clear benefit, as providing an implicit guarantee on static initialization order would run the risk of constraining future development and making it more difficult.)

In the meantime, if you really need this locally, the patch is relatively simple right now. You can simply replace

MockObjectRegistry g_mock_object_registry;

with

MockObjectRegistry* GetMockObjectRegistry() {
  static std::atomic<MockObjectRegistry*> g_instance;
  MockObjectRegistry* instance = g_instance.load(std::memory_order_relaxed);
  if (instance == nullptr) {
    internal::MutexLock l(&internal::g_gmock_mutex);
    instance = new MockObjectRegistry();
    g_instance.store(instance, std::memory_order_relaxed);
    ::atexit([]() { delete g_instance.load(std::memory_order_relaxed); });
  }
  return instance;
}

// Ensures initialization occurs at the same time as other globals or earlier.
std::nullptr_t g_mock_object_ctor = ((void)GetMockObjectRegistry(), nullptr);

and then find/replace all uses of g_mock_object_registry with GetMockObjectRegistry().