diff --git a/test-app/app/src/main/assets/app/mainpage.js b/test-app/app/src/main/assets/app/mainpage.js index 3996a5d75..e20563c01 100644 --- a/test-app/app/src/main/assets/app/mainpage.js +++ b/test-app/app/src/main/assets/app/mainpage.js @@ -34,6 +34,7 @@ require("./tests/testMultithreadedJavascript"); require("./tests/testWorkerTerminateDuringLoad"); require("./tests/testWorkerOptions"); require("./tests/testWorkerResourceLimits"); +require("./tests/testWorkerNestedConsole"); require("./tests/testMessaging"); require("./tests/testInterfaceDefaultMethods"); require("./tests/testInterfaceStaticMethods"); diff --git a/test-app/app/src/main/assets/app/tests/testWorkerNestedConsole.js b/test-app/app/src/main/assets/app/tests/testWorkerNestedConsole.js new file mode 100644 index 000000000..4baaa1697 --- /dev/null +++ b/test-app/app/src/main/assets/app/tests/testWorkerNestedConsole.js @@ -0,0 +1,15 @@ +describe("Worker nested console call", function () { + it("should keep running when formatting a logged argument logs again", function (done) { + var worker = new Worker("./workerNestedConsoleWorker.js"); + // A worker stuck in ConsoleLog holds the inspector lock that + // terminate() takes, so only the success path may terminate it. + worker.onmessage = function (e) { + expect(e.data).toBe("alive"); + worker.terminate(); + done(); + }; + worker.onerror = function (e) { + done.fail(e.message); + }; + }); +}); diff --git a/test-app/app/src/main/assets/app/tests/workerNestedConsoleWorker.js b/test-app/app/src/main/assets/app/tests/workerNestedConsoleWorker.js new file mode 100644 index 000000000..9eac26f53 --- /dev/null +++ b/test-app/app/src/main/assets/app/tests/workerNestedConsoleWorker.js @@ -0,0 +1,11 @@ +// The inspector reads Symbol.toStringTag while it formats a console argument, +// so this getter runs inside the outer console.log. +var arg = {}; +Object.defineProperty(arg, Symbol.toStringTag, { + get: function () { + console.log("nested"); + return "Nested"; + } +}); +console.log(arg); +postMessage("alive"); diff --git a/test-app/runtime/src/main/cpp/JsV8InspectorClient.cpp b/test-app/runtime/src/main/cpp/JsV8InspectorClient.cpp index d1ef7be83..5beb11a14 100644 --- a/test-app/runtime/src/main/cpp/JsV8InspectorClient.cpp +++ b/test-app/runtime/src/main/cpp/JsV8InspectorClient.cpp @@ -1134,9 +1134,9 @@ void JsV8InspectorClient::sendToFrontEndCallback(const v8::FunctionCallbackInfo< } void JsV8InspectorClient::consoleLogCallback(Isolate* isolate, ConsoleAPIType method, const std::vector>& args) { - // Worker isolates route to their own inspector (we're on the worker's - // thread here). Checked first so worker logging never lazily constructs - // the root client. + // Worker isolates route to their own inspector (from the worker thread, + // or the main thread under __runOnMainThread). Checked first so worker + // logging never lazily constructs the root client. WorkerWrapper* worker = WorkerWrapper::FromIsolate(isolate); if (worker != nullptr) { worker->ConsoleLog(method, args); diff --git a/test-app/runtime/src/main/cpp/WorkerInspectorClient.h b/test-app/runtime/src/main/cpp/WorkerInspectorClient.h index a3deb572a..6cfcee6b2 100644 --- a/test-app/runtime/src/main/cpp/WorkerInspectorClient.h +++ b/test-app/runtime/src/main/cpp/WorkerInspectorClient.h @@ -74,8 +74,9 @@ class WorkerInspectorClient final : public v8_inspector::V8InspectorClient, return runningPauseLoop_.load(std::memory_order_acquire); } - // Worker thread. Mirrors JsV8InspectorClient::consoleLogCallback for this - // isolate. + // Thread holding the worker isolate's Locker (the worker thread, or the + // main thread under __runOnMainThread). Mirrors + // JsV8InspectorClient::consoleLogCallback for this isolate. void consoleLog(v8_inspector::ConsoleAPIType method, const std::vector>& args); diff --git a/test-app/runtime/src/main/cpp/WorkerWrapper.cpp b/test-app/runtime/src/main/cpp/WorkerWrapper.cpp index ae349b282..f18f137c2 100644 --- a/test-app/runtime/src/main/cpp/WorkerWrapper.cpp +++ b/test-app/runtime/src/main/cpp/WorkerWrapper.cpp @@ -259,7 +259,7 @@ void WorkerWrapper::Terminate() { // A worker paused at a breakpoint sits in the inspector's nested pause // loop, not in Looper.loop() - kick it loose so TerminateExecution and // the looper quit below can take effect. - std::lock_guard lock(inspectorMutex_); + std::lock_guard lock(inspectorMutex_); if (inspector_ != nullptr) { inspector_->NotifyTerminating(); } @@ -930,7 +930,7 @@ void WorkerWrapper::CreateInspector(Isolate* isolate) { auto* client = new WorkerInspectorClient(workerId_, isolate, ALooper_forThread(), url); { - std::lock_guard lock(inspectorMutex_); + std::lock_guard lock(inspectorMutex_); inspector_ = client; } @@ -942,7 +942,7 @@ void WorkerWrapper::CreateInspector(Isolate* isolate) { void WorkerWrapper::DestroyInspector() { WorkerInspectorClient* client = nullptr; { - std::lock_guard lock(inspectorMutex_); + std::lock_guard lock(inspectorMutex_); client = inspector_; inspector_ = nullptr; } @@ -963,7 +963,7 @@ void WorkerWrapper::DestroyInspector() { void WorkerWrapper::ConsoleLog(v8_inspector::ConsoleAPIType method, const std::vector>& args) { - std::lock_guard lock(inspectorMutex_); + std::lock_guard lock(inspectorMutex_); if (inspector_ != nullptr) { inspector_->consoleLog(method, args); } diff --git a/test-app/runtime/src/main/cpp/WorkerWrapper.h b/test-app/runtime/src/main/cpp/WorkerWrapper.h index ba1e74184..c130e6f90 100644 --- a/test-app/runtime/src/main/cpp/WorkerWrapper.h +++ b/test-app/runtime/src/main/cpp/WorkerWrapper.h @@ -166,8 +166,10 @@ class WorkerWrapper : public std::enable_shared_from_this { #ifdef APPLICATION_IN_DEBUG /* - * Worker thread (console.* fires on the isolate's own thread). Forwards - * to the worker's inspector so the log reaches DevTools' worker target. + * Any thread holding the worker isolate's Locker: the worker thread, or + * the main thread while it runs a worker callback through + * __runOnMainThread. Forwards to the worker's inspector so the log + * reaches DevTools' worker target. */ void ConsoleLog(v8_inspector::ConsoleAPIType method, const std::vector>& args); @@ -253,7 +255,13 @@ class WorkerWrapper : public std::enable_shared_from_this { void DestroyInspector(); WorkerInspectorClient* inspector_ = nullptr; - std::mutex inspectorMutex_; + // Held across consoleLog, which allocates and runs user JS while it + // stringifies the arguments; releasing it there would let the worker + // thread delete the client under a __runOnMainThread caller. Recursive + // because that call re-enters on the same thread: a nested console call + // from a getter, or a near-heap-limit callback raised by an allocation, + // which calls Terminate(). + std::recursive_mutex inspectorMutex_; #endif static std::mutex registryMutex_;