Conversation
WalkthroughReplaces Node-based scripting with a standalone V8Engine: adds V8 engine sources, callbacks, and CMake object target; migrates client scripting module to use V8Engine; moves engine execution/SDK/helpers into base Engine; updates module loading, timers, and promise handling paths. Changes
Sequence DiagramssequenceDiagram
participant Client as Client Scripting Module
participant Engine as V8Engine
participant Platform as V8 Platform
participant Context as V8 Context
participant Module as Module System
Client->>Engine: SetModuleRootPath(path)
Client->>Engine: Init()
Engine->>Platform: InitializePlatform()
Engine->>Engine: CreateIsolateAndContext()
Engine->>Context: InstallGlobals (timers, require, queueMicrotask)
Context->>Module: Prepare module system
Engine-->>Client: Initialized
Client->>Engine: ExecuteFile(scriptPath)
Engine->>Module: ResolveModulePath(requested, fromDir)
Module->>Module: Read & compile module
Module->>Context: Execute wrapped code
Module-->>Engine: module.exports
Engine-->>Client: Success / Error
sequenceDiagram
participant Script as User Script
participant Require as require()
participant Engine as V8Engine
participant Cache as Module Cache
participant Isolate as V8 Isolate
Script->>Require: require("mod")
Require->>Engine: LoadModule(requested, currentDir)
Engine->>Cache: Check cache for resolved path
alt cached
Cache-->>Require: cached exports
else not cached
Engine->>Isolate: Compile & execute module
Isolate->>Engine: module.exports
Engine->>Cache: Store exports
Engine-->>Require: exports
end
Require-->>Script: exports
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@code/framework/src/integrations/client/scripting/module.cpp`:
- Around line 30-47: ClientScriptingModule::Init can dereference a null _engine
if called after Shutdown() (which sets _engine to nullptr); add a defensive null
check at the top of Init to avoid calling _engine->SetSDKRegisterCallback,
_engine->SetModuleRootPath or _engine->Init when _engine is null — either
recreate _engine before use if Init-after-Shutdown is a supported lifecycle, or
log an error and return false immediately (include a clear message referencing
Shutdown) so subsequent calls like SetSDKRegisterCallback, SetModuleRootPath and
_engine->Init are never invoked on a null pointer; ensure the check references
_engine, Shutdown(), Reset(), SetSDKRegisterCallback, SetModuleRootPath and
_engine->Init to locate the change.
In `@code/framework/src/scripting/v8_engine_callbacks.h`:
- Around line 21-37: ReadFileContents currently returns true without verifying
that file.read() actually read the requested number of bytes; update
ReadFileContents to check the read result (e.g., check file.fail() or that
file.gcount() == size) after file.read(outContent.data(), size), and if the read
failed or is partial, log a warning via
Logging::GetLogger(FRAMEWORK_INNER_SCRIPTING) including filepath and byte
counts, clear or reset outContent, and return false; keep existing size checks
(kMaxModuleFileSize) and behavior otherwise.
In `@code/framework/src/scripting/v8_engine.cpp`:
- Around line 245-296: ProcessTimers currently collects raw TimerEntry* into
dueTimers which can dangle if AddTimer pushes and reallocates _timers; change
the collection to store stable identifiers or indices and look up the timer from
_timers at call time. Specifically, in V8Engine::ProcessTimers replace
std::vector<TimerEntry*> dueTimers with std::vector<size_t> dueIndices (or use
timer->id values if TimerEntry has an ID), push the index (or id) for each due
timer when scanning _timers, and when invoking the callback retrieve the timer
via _timers[index].get() (or map id→pointer) and re-check cancelled/bounds
before use; optionally also keep a local shared_ptr/unique_ptr copy or reserve
_timers capacity in AddTimer/constructor to prevent reallocation as an
additional mitigation.
- Around line 298-357: The path boundary check in V8Engine::ResolveModulePath
uses case-sensitive string comparison (resolvedStr vs rootStr) but on Windows
filesystems are case-insensitive; normalize both paths to a consistent case on
Windows before the comparison to avoid sandbox bypass. Modify the logic around
resolvedStr/rootStr (used with _options.moduleRootPath) to, when
compiling/running on Windows, lowercase (or otherwise case-normalize) both root
and resolved path strings (after weakly_canonical) and then perform the existing
prefix + boundary checks; keep the existing checks for non-Windows unchanged.
🧹 Nitpick comments (6)
code/framework/src/scripting/engine.cpp (2)
14-19: Inconsistent variable naming: use camelCase per project guidelines.Lines 16-17 and 19 use
snake_case(isolate_scope,handle_scope,context_scope) while line 21 usescamelCase(tryCatch). The coding guidelines specify camelCase for variables.🔧 Suggested naming fix
v8::Isolate *isolate = GetIsolate(); v8::Locker locker(isolate); - v8::Isolate::Scope isolate_scope(isolate); - v8::HandleScope handle_scope(isolate); + v8::Isolate::Scope isolateScope(isolate); + v8::HandleScope handleScope(isolate); v8::Local<v8::Context> context = GetContext(); - v8::Context::Scope context_scope(context); + v8::Context::Scope contextScope(context);As per coding guidelines: "Use
_prefix for private members and camelCase naming for variables."
56-71:GetFrameworkObject()assumes V8 scopes are already active — consider documenting this precondition.This method is public and
const, but it accessesGetContext()->Global()without establishing aLocker/HandleScope. Callers must set up V8 scopes beforehand. The current call sites appear to do so, but a brief doc comment on the declaration (inengine.hline 96-99) would make this contract explicit.code/framework/src/scripting/v8_engine_callbacks.h (1)
15-18: Nit:staticis redundant onconstexprnamespace-scope variables in C++20.
constexprvariables at namespace scope already have internal linkage. Thestatickeyword is harmless but redundant.Suggested fix
- static constexpr std::streamsize kMaxModuleFileSize = 10 * 1024 * 1024; + constexpr std::streamsize kMaxModuleFileSize = 10 * 1024 * 1024;code/framework/src/scripting/v8_engine.h (2)
78-102: Consider reducing public API surface for internal callback data.
TimerEntry,RequireData, andTimerCallbackDataare public solely to be accessible from anonymous-namespace callbacks inv8_engine_callbacks.h. This exposes implementation details in the public header. Afrienddeclaration or moving these to an internal header could tighten the interface, but this is a minor encapsulation nit given the current architecture.
122-123:_nextTimerIdwill wrap around to 0 afterUINT32_MAXtimers.After ~4 billion timer creations,
_nextTimerId++wraps to 0.AddTimerassignsid = _nextTimerId++, so ID 0 would be assigned, and subsequent IDs could collide with still-active timers. Practically near-impossible to hit, but if you want belt-and-suspenders:🔧 Optional: skip ID 0 and check for collisions
uint32_t V8Engine::AddTimer(std::unique_ptr<TimerEntry> entry) { if (_timers.size() >= kMaxTimers) { Logging::GetLogger(FRAMEWORK_INNER_SCRIPTING)->warn("Timer limit ({}) reached, ignoring new timer", kMaxTimers); return 0; } - uint32_t id = _nextTimerId++; + uint32_t id = _nextTimerId++; + if (id == 0) id = _nextTimerId++; // skip 0 (used as "no timer" sentinel) entry->id = id; _timers.push_back(std::move(entry)); return id; }code/framework/src/scripting/v8_engine.cpp (1)
225-234:AddTimerreturns 0 on limit — ensure callers treat 0 as "no timer".
SetTimerCallback(in v8_engine_callbacks.h) returns the timer ID to JavaScript viaargs.GetReturnValue().Set(...). When the limit is reached, ID 0 is returned. Per the Web API spec,setTimeout/setIntervalshould always return a positive integer. Returning 0 could confuse scripts that useif (timerId)checks, andclearTimeout(0)would be a no-op (since no timer has ID 0), so this is mostly benign — but worth being aware of.
…ework into scripting/security-harness
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@code/framework/src/integrations/client/scripting/module.cpp`:
- Around line 46-48: In Init(), avoid calling std::filesystem::absolute on an
empty _resourceCachePath because that resolves to cwd; instead check
_resourceCachePath first (from SetResourceCachePath) and if it's empty call
_engine->SetModuleRootPath("") (or log/throw a clear error) rather than
std::filesystem::absolute(_resourceCachePath). Update the code around
SetModuleRootPath to use this guard so require() is sandboxed correctly or the
missing cache path fails fast; reference Init, _resourceCachePath,
SetResourceCachePath, and SetModuleRootPath when making the change.
🧹 Nitpick comments (5)
code/framework/src/scripting/v8_engine.cpp (5)
16-17: Static platform initialization is not thread-safe.
_platformInitializedand_platformare static members accessed without synchronization. If twoV8Engineinstances are ever constructed concurrently (from different threads),InitializePlatform()can race — both threads see_platformInitialized == false, andv8::V8::Initialize()gets called twice, which is undefined behavior per V8 docs.If only one engine instance exists in practice, this is not an active bug, but a
std::once_flag/std::call_oncewould be a low-cost safeguard:Suggested fix
- std::unique_ptr<v8::Platform> V8Engine::_platform = nullptr; - bool V8Engine::_platformInitialized = false; + std::unique_ptr<v8::Platform> V8Engine::_platform = nullptr; + std::once_flag V8Engine::_platformInitFlag; // ... bool V8Engine::InitializePlatform() { - if (_platformInitialized) { - return true; - } - - _platform = v8::platform::NewDefaultPlatform(); - v8::V8::InitializePlatform(_platform.get()); - v8::V8::Initialize(); - - _platformInitialized = true; + std::call_once(_platformInitFlag, []() { + _platform = v8::platform::NewDefaultPlatform(); + v8::V8::InitializePlatform(_platform.get()); + v8::V8::Initialize(); + }); return true; }Also applies to: 91-103
53-89: Consider holding the V8 Locker while clearing persistent handles.Lines 69–75 destroy
v8::Globalhandles (inside_timersand_moduleCache) after thev8::Lockerblock at lines 58–66 has exited. Whilev8::Globaldestructors typically don't require the locker, some V8 embedder guides recommend holding it during destruction if the isolate is still alive, to avoid races with concurrent GC or platform tasks.Moving the
.clear()calls inside the existing locker block (or extending it) would be a low-risk safety improvement:Suggested adjustment
{ v8::Locker locker(_isolate); v8::Isolate::Scope isolate_scope(_isolate); v8::HandleScope handle_scope(_isolate); v8::Local<v8::Context> context = _context.Get(_isolate); v8::Context::Scope context_scope(context); Messages::Shutdown(); + + // Clear persistent handles while holding the locker + _timers.clear(); + _moduleCache.clear(); + _requireDataStore.clear(); + _context.Reset(); } - // Clear all timer callbacks (prevent dangling references) - _timers.clear(); - - // Clear module cache - _moduleCache.clear(); - - // Clear require data store (frees all RequireData instances) - _requireDataStore.clear(); - - // Reset the persistent context handle - _context.Reset(); - // Dispose the isolate _isolate->Dispose();
227-236: Timer ID 0 serves double duty as both error sentinel and potential valid ID after wrap-around.
AddTimerreturns0to signal "limit reached" (line 230), but_nextTimerId++will eventually wrap to0after ~4 billion timers, making a successfully added timer indistinguishable from the error case. Practically unreachable in a normal session, but starting the counter at1would eliminate the ambiguity cheaply:Suggested tweak
uint32_t id = _nextTimerId++; + if (id == 0) id = _nextTimerId++; // skip sentinel value entry->id = id;
58-66: Local variable naming usessnake_caseinstead ofcamelCase.Variables like
isolate_scope,handle_scope, andcontext_scopethroughoutv8_engine.cppusesnake_case, whereas the coding guidelines specify camelCase for variables (andmodule.cppconsistently usesisolateScope,handleScope,contextScope).Example diff (apply throughout)
- v8::Locker locker(_isolate); - v8::Isolate::Scope isolate_scope(_isolate); - v8::HandleScope handle_scope(_isolate); + v8::Locker locker(_isolate); + v8::Isolate::Scope isolateScope(_isolate); + v8::HandleScope handleScope(_isolate);As per coding guidelines: "Use
_prefix for private members and camelCase naming for variables."Also applies to: 511-514
91-103: V8 platform is initialized but never disposed.
InitializePlatform()callsv8::V8::Initialize()but there's no correspondingv8::V8::Dispose()/v8::V8::ShutdownPlatform()anywhere. For a game client that runs until process exit this is typically fine (OS reclaims resources), but it means the V8 platform is effectively process-lifetime. If clean restart of the scripting subsystem is ever needed without restarting the process, this would need to be addressed.
github-project-automation
bot
moved this from In Progress
to Done
in MafiaHub Dashboard
2 participants
Client side scripting shouldn't use nodejs for security reason, but only raw V8 integration.
This PR fixes that.
Summary by CodeRabbit
New Features
Chores