From a301af92dd8a915518554e68824e72ca98b56a8f Mon Sep 17 00:00:00 2001 From: Tomasz Karczewski Date: Mon, 12 Aug 2024 12:41:14 +0200 Subject: [PATCH] [2.38] limit conservative scan range Turns out that conservative GC root scanning is going too deep into the stack and this may prevent the collection of objects - see: https://github.com/WebPlatformForEmbedded/WPEWebKit/issues/1363 The change introduces 'stack guards' that prevent this in particular cases, that is - if there is gloop main loop routine in the stack the GC root stack search will not go deeper that this frame. This avoids some 'false positives' conservative roots that can make the app hold a lot of memory. --- .../heap/MachineStackMarker.cpp | 3 +- Source/WTF/wtf/CMakeLists.txt | 2 + .../WTF/wtf/ConservativeScanStackGuards.cpp | 6 ++ Source/WTF/wtf/ConservativeScanStackGuards.h | 65 +++++++++++++++++++ Source/WTF/wtf/glib/RunLoopGLib.cpp | 3 + Source/WebKit/WebProcess/WebProcess.cpp | 4 ++ 6 files changed, 82 insertions(+), 1 deletion(-) create mode 100644 Source/WTF/wtf/ConservativeScanStackGuards.cpp create mode 100644 Source/WTF/wtf/ConservativeScanStackGuards.h diff --git a/Source/JavaScriptCore/heap/MachineStackMarker.cpp b/Source/JavaScriptCore/heap/MachineStackMarker.cpp index 99a70fdf3efcd..c914f07c29286 100644 --- a/Source/JavaScriptCore/heap/MachineStackMarker.cpp +++ b/Source/JavaScriptCore/heap/MachineStackMarker.cpp @@ -27,6 +27,7 @@ #include #include #include +#include namespace JSC { @@ -44,7 +45,7 @@ void MachineThreads::gatherFromCurrentThread(ConservativeRoots& conservativeRoot conservativeRoots.add(registersBegin, registersEnd, jitStubRoutines, codeBlocks); } - conservativeRoots.add(currentThreadState.stackTop, currentThreadState.stackOrigin, jitStubRoutines, codeBlocks); + conservativeRoots.add(currentThreadState.stackTop, WTF::ConservativeScanStackGuards::updatedStackOrigin(currentThreadState.stackTop, currentThreadState.stackOrigin), jitStubRoutines, codeBlocks); } static inline int osRedZoneAdjustment() diff --git a/Source/WTF/wtf/CMakeLists.txt b/Source/WTF/wtf/CMakeLists.txt index 826237ba339d3..f2a0f1d7c6431 100644 --- a/Source/WTF/wtf/CMakeLists.txt +++ b/Source/WTF/wtf/CMakeLists.txt @@ -46,6 +46,7 @@ set(WTF_PUBLIC_HEADERS ConcurrentPtrHashSet.h ConcurrentVector.h Condition.h + ConservativeScanStackGuards.h CountingLock.h CrossThreadCopier.h CrossThreadQueue.h @@ -422,6 +423,7 @@ set(WTF_SOURCES CompilationThread.cpp ConcurrentBuffer.cpp ConcurrentPtrHashSet.cpp + ConservativeScanStackGuards.cpp CountingLock.cpp CrossThreadCopier.cpp CrossThreadTaskHandler.cpp diff --git a/Source/WTF/wtf/ConservativeScanStackGuards.cpp b/Source/WTF/wtf/ConservativeScanStackGuards.cpp new file mode 100644 index 0000000000000..06030a0d61081 --- /dev/null +++ b/Source/WTF/wtf/ConservativeScanStackGuards.cpp @@ -0,0 +1,6 @@ +#include "ConservativeScanStackGuards.h" + +namespace WTF { + std::atomic_bool ConservativeScanStackGuards::active {true}; + thread_local std::deque ConservativeScanStackGuards::guard_ptr_stack; +} diff --git a/Source/WTF/wtf/ConservativeScanStackGuards.h b/Source/WTF/wtf/ConservativeScanStackGuards.h new file mode 100644 index 0000000000000..1216474ce0dfb --- /dev/null +++ b/Source/WTF/wtf/ConservativeScanStackGuards.h @@ -0,0 +1,65 @@ +#pragma once + +#include "Logging.h" + +#include +#include + +namespace WTF { + class ConservativeScanStackGuards { + public: + struct ConservativeScanStackGuard { + ConservativeScanStackGuard() { + ConservativeScanStackGuards::setStackGuard(this); + } + ~ConservativeScanStackGuard() { + ConservativeScanStackGuards::resetStackGuard(this); + } + + private: + // Prevent heap allocation + void *operator new (size_t); + void *operator new[] (size_t); + void operator delete (void *); + void operator delete[] (void*); + }; + + friend struct ConservativeScanStackGuard; + + static void* updatedStackOrigin(void *stackTop, void *stackOrigin) { + if (!active.load() || guard_ptr_stack.empty()) { + return stackOrigin; + } + + void *ret = stackOrigin; + + void *guard_ptr = guard_ptr_stack.back(); + + if (guard_ptr > stackTop && guard_ptr < stackOrigin) { + WTFLogAlways("ConservativeScanStackGuards: guard IN RANGE: stackTop: %p guard: %p stackOrigin: %p; correcting stackOrigin\n", stackTop, guard_ptr, stackOrigin); + ret = guard_ptr; + } + return ret; + } + + static void setActive(bool act) { + active.store(act); + } + + private: + + static thread_local std::deque guard_ptr_stack; + static std::atomic_bool active; + + static void setStackGuard(void *ptr) { + guard_ptr_stack.push_back(ptr); + } + + static void resetStackGuard(void *ptr) { + if (ptr != guard_ptr_stack.back()) { + WTFLogAlways("ConservativeScanStackGuards::resetStackGuard expected %p, had: %p", guard_ptr_stack.back(), ptr); + } + guard_ptr_stack.pop_back(); + } + }; +} \ No newline at end of file diff --git a/Source/WTF/wtf/glib/RunLoopGLib.cpp b/Source/WTF/wtf/glib/RunLoopGLib.cpp index 027aa10964f12..dc839b89d6273 100644 --- a/Source/WTF/wtf/glib/RunLoopGLib.cpp +++ b/Source/WTF/wtf/glib/RunLoopGLib.cpp @@ -31,6 +31,8 @@ #include #include +#include "wtf/ConservativeScanStackGuards.h" + namespace WTF { typedef struct { @@ -103,6 +105,7 @@ void RunLoop::run() ASSERT(!runLoop.m_mainLoops.isEmpty()); GMainLoop* innermostLoop = runLoop.m_mainLoops[0].get(); + ConservativeScanStackGuards::ConservativeScanStackGuard guard; if (!g_main_loop_is_running(innermostLoop)) { g_main_context_push_thread_default(mainContext); g_main_loop_run(innermostLoop); diff --git a/Source/WebKit/WebProcess/WebProcess.cpp b/Source/WebKit/WebProcess/WebProcess.cpp index c045c461b479e..da235c776cc44 100644 --- a/Source/WebKit/WebProcess/WebProcess.cpp +++ b/Source/WebKit/WebProcess/WebProcess.cpp @@ -1042,6 +1042,10 @@ void WebProcess::isJITEnabled(CompletionHandler&& completionHandler) void WebProcess::garbageCollectJavaScriptObjects() { + { + JSLockHolder lock(commonVM()); + commonVM().shrinkFootprintWhenIdle(); + } GCController::singleton().garbageCollectNow(); }