Skip to content

Commit dd75619

Browse files
bghgaryCopilot
andcommitted
Add regression test for ExternalTexture::CreateForJavaScript recursive-mutex deadlock
Reproduce the deadlock reported in #1646 (comment) without needing any Babylon scene, GC pressure, or external scheduling. The bug: CreateForJavaScript holds m_impl->Mutex() across the JS-side property lookup `Graphics::DeviceContext::GetFromJavaScript(env)`. That lookup can run user JS or engine GC/finalizers (e.g. a sibling Texture finalizer registered by Napi::Pointer::Create) which themselves re-enter m_impl->Mutex() on the same thread. On MSVC, recursively locking std::mutex throws std::system_error("resource_deadlock_would_occur"), which then escapes the AppRuntime dispatch lambda and triggers std::abort. The test redefines `_native._Graphics` as an accessor whose getter, when invoked, asks the ExternalTexture for its Width() (which also takes m_impl->Mutex()) and observes whether the lock is currently held on the caller's thread. With the bug present, Width() throws system_error and the test reports `recursiveLockObserved=true`. With the fix, the lookup runs before the mutex is acquired and Width() succeeds. Test is Win32-only because it relies on MSVC's std::mutex deadlock detection to surface the recursive lock as a recoverable exception. [Created by Copilot on behalf of @bghgary] Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent e453e9d commit dd75619

1 file changed

Lines changed: 128 additions & 0 deletions

File tree

Apps/UnitTests/Source/Tests.ExternalTexture.cpp

Lines changed: 128 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,9 @@
99

1010
#include "Utils.h"
1111

12+
#include <atomic>
1213
#include <iostream>
14+
#include <system_error>
1315

1416
extern Babylon::Graphics::Configuration g_deviceConfig;
1517

@@ -78,6 +80,132 @@ TEST(ExternalTexture, CreateForJavaScript)
7880
#endif
7981
}
8082

83+
// Regression test for the recursive-lock deadlock in ExternalTexture::CreateForJavaScript:
84+
// the implementation must not hold m_impl->Mutex() while it makes a JS-side property lookup
85+
// (Graphics::DeviceContext::GetFromJavaScript), because that lookup can run user JS or
86+
// engine GC/finalizers that may re-enter the same mutex from the same thread.
87+
//
88+
// This test redefines `_Graphics` on the global `_native` object as an accessor whose
89+
// getter calls back into native code and asks the ExternalTexture for its Width(), which
90+
// also takes m_impl->Mutex(). With the bug present, the lookup runs while the mutex is
91+
// already held on this thread, so Width()'s scoped_lock throws
92+
// system_error("resource_deadlock_would_occur") on MSVC. With the fix, the lookup runs
93+
// before the mutex is taken, and Width() succeeds.
94+
//
95+
// The test is restricted to Win32 (Chakra + MSVC std::mutex deadlock detection).
96+
TEST(ExternalTexture, CreateForJavaScriptDoesNotHoldImplMutexAcrossJsCallout)
97+
{
98+
#if defined(SKIP_EXTERNAL_TEXTURE_TESTS) || !defined(_WIN32)
99+
GTEST_SKIP();
100+
#else
101+
Babylon::Graphics::Device device{g_deviceConfig};
102+
Babylon::Graphics::DeviceUpdate update{device.GetUpdate("update")};
103+
104+
device.StartRenderingCurrentFrame();
105+
update.Start();
106+
107+
auto nativeTexture = CreateTestTexture(device.GetPlatformInfo().Device, 256, 256);
108+
Babylon::Plugins::ExternalTexture externalTexture{nativeTexture};
109+
DestroyTestTexture(nativeTexture);
110+
111+
std::promise<std::exception_ptr> done{};
112+
std::atomic<bool> probeGetterHit{false};
113+
std::atomic<bool> recursiveLockObserved{false};
114+
115+
Babylon::AppRuntime runtime{};
116+
runtime.Dispatch([&device, &done, &probeGetterHit, &recursiveLockObserved, externalTexture](Napi::Env env) {
117+
try
118+
{
119+
device.AddToJavaScript(env);
120+
121+
Babylon::Polyfills::Console::Initialize(env, [](const char* message, auto) {
122+
std::cout << message << std::endl;
123+
});
124+
125+
Babylon::Polyfills::Window::Initialize(env);
126+
127+
Babylon::Plugins::NativeEngine::Initialize(env);
128+
129+
// Stash a reference to the real _Graphics value, then redefine _native._Graphics
130+
// as an accessor whose getter (a) probes the ExternalTexture lock and (b) returns
131+
// the real value. This intercepts the same lookup that
132+
// Graphics::DeviceContext::GetFromJavaScript does inside CreateForJavaScript.
133+
auto nativeObj = env.Global().Get("_native").As<Napi::Object>();
134+
env.Global().Set("__realGraphicsForTest", nativeObj.Get("_Graphics"));
135+
136+
auto graphicsGetter = Napi::Function::New(env,
137+
[tex = externalTexture, &probeGetterHit, &recursiveLockObserved](const Napi::CallbackInfo& info) -> Napi::Value {
138+
probeGetterHit = true;
139+
try
140+
{
141+
// Width() also takes m_impl->Mutex(). If CreateForJavaScript is
142+
// currently holding it on this thread, this re-entrant lock attempt
143+
// throws system_error on MSVC's std::mutex.
144+
static_cast<void>(tex.Width());
145+
}
146+
catch (const std::system_error&)
147+
{
148+
recursiveLockObserved = true;
149+
}
150+
catch (const std::exception&)
151+
{
152+
recursiveLockObserved = true;
153+
}
154+
return info.Env().Global().Get("__realGraphicsForTest");
155+
});
156+
157+
napi_property_descriptor desc{};
158+
desc.utf8name = "_Graphics";
159+
desc.getter = [](napi_env e, napi_callback_info cbinfo) -> napi_value {
160+
size_t argc = 0;
161+
napi_value thisArg = nullptr;
162+
void* data = nullptr;
163+
napi_get_cb_info(e, cbinfo, &argc, nullptr, &thisArg, &data);
164+
napi_value getter = static_cast<napi_value>(data);
165+
napi_value result = nullptr;
166+
napi_call_function(e, thisArg, getter, 0, nullptr, &result);
167+
return result;
168+
};
169+
desc.attributes = static_cast<napi_property_attributes>(napi_configurable);
170+
desc.data = static_cast<napi_value>(graphicsGetter);
171+
napi_status status = napi_define_properties(env, nativeObj, 1, &desc);
172+
if (status != napi_ok)
173+
{
174+
throw std::runtime_error{"napi_define_properties failed installing test getter"};
175+
}
176+
177+
externalTexture.CreateForJavaScript(env);
178+
179+
done.set_value(nullptr);
180+
}
181+
catch (...)
182+
{
183+
done.set_value(std::current_exception());
184+
}
185+
});
186+
187+
auto future = done.get_future();
188+
ASSERT_EQ(future.wait_for(std::chrono::seconds(60)), std::future_status::ready)
189+
<< "Dispatch did not complete in time; possible deadlock in CreateForJavaScript.";
190+
191+
if (auto ex = future.get())
192+
{
193+
std::rethrow_exception(ex);
194+
}
195+
196+
EXPECT_TRUE(probeGetterHit.load())
197+
<< "Test getter for _native._Graphics was not invoked; "
198+
<< "the test did not actually exercise the lock-holding window.";
199+
EXPECT_FALSE(recursiveLockObserved.load())
200+
<< "ExternalTexture::CreateForJavaScript held m_impl->Mutex() across the JS-side "
201+
<< "Graphics::DeviceContext::GetFromJavaScript lookup. This can deadlock or terminate "
202+
<< "the process when the JS callout triggers a finalizer that re-enters the same mutex.";
203+
204+
update.Finish();
205+
device.FinishRenderingCurrentFrame();
206+
#endif
207+
}
208+
81209
TEST(ExternalTexture, Update)
82210
{
83211
#ifdef SKIP_EXTERNAL_TEXTURE_TESTS

0 commit comments

Comments
 (0)