From b053cd45ec7b8904374daedaf2cd10fd8d79c027 Mon Sep 17 00:00:00 2001 From: Christoph Schaefer Date: Thu, 27 Aug 2026 10:43:40 +0200 Subject: [PATCH 1/3] fix(doctrenderer): don't abort the process when a script fails to compile CCacheDataScript::Compile()'s cache-generation path called ToLocalChecked() on the result of ScriptCompiler::Compile() without checking it first, unlike the three sibling compile paths in the same function. A JS syntax error therefore became a V8 CHECK failure -- an abort (SIGILL/SIGTRAP) whose only trace is "Fatal error in v8::ToLocalChecked / Empty MaybeLocal", with the underlying SyntaxError never reported anywhere. Two changes: - Check the MaybeLocal before unwrapping it and return the empty script, so the caller's CJSTryCatch surfaces the real error instead of the process dying. - Create the .cache file only once CreateCodeCache() has actually produced data. Creating it up front left a zero-length .cache behind whenever the compile aborted; the next run then took the Exists(Path) branch and handed that empty buffer to kConsumeCodeCache. Surfaced by Euro-Office/sdkjs#80, where Terser-emitted supplementary-plane identifiers made the sdkjs bundles unparsable for this no-ICU V8 build (see Common/3dParty/v8/tools/8.9/*/nc-build.sh, v8_enable_i18n_support=false). The bundles are fixed on the sdkjs side; this makes any future occurrence diagnosable rather than fatal. The pre-V8-8.9 branch (#else) carries the same unguarded pattern. It is not built by this repo's toolchain, so it is left untouched rather than changed without a way to compile-test it. Assisted-by: ClaudeCode:claude-opus-5 Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Christoph Schaefer --- .../doctrenderer/js_internal/v8/v8_base.cpp | 33 +++++++++++++------ 1 file changed, 23 insertions(+), 10 deletions(-) diff --git a/DesktopEditor/doctrenderer/js_internal/v8/v8_base.cpp b/DesktopEditor/doctrenderer/js_internal/v8/v8_base.cpp index 5192bda717..d99a595632 100644 --- a/DesktopEditor/doctrenderer/js_internal/v8/v8_base.cpp +++ b/DesktopEditor/doctrenderer/js_internal/v8/v8_base.cpp @@ -86,18 +86,31 @@ namespace NSJSBase { v8::ScriptCompiler::CachedData* pCacheData = nullptr; - // save cache to file - NSFile::CFileBinary oFileTest; - if (oFileTest.CreateFileW(Path)) - { - // create cache data - v8::ScriptCompiler::Source oSource(source); - v8::Local pScriptCache = v8::ScriptCompiler::Compile(_context, &oSource, v8::ScriptCompiler::kNoCompileOptions).ToLocalChecked(); - pCacheData = v8::ScriptCompiler::CreateCodeCache(pScriptCache->GetUnboundScript()); + // Compile once to produce the code cache. + // + // The result must be checked before ToLocalChecked(): a JS syntax error + // makes Compile() return an empty MaybeLocal, and ToLocalChecked() turns + // that recoverable parse error into a V8 CHECK failure, i.e. an abort + // (SIGILL/SIGTRAP) whose only trace is "Fatal error in v8::ToLocalChecked + // / Empty MaybeLocal". Returning the empty script instead lets the + // caller's CJSTryCatch report the actual SyntaxError, matching the three + // sibling compile paths in this function. + v8::ScriptCompiler::Source oSource(source); + v8::MaybeLocal scriptCacheMB = v8::ScriptCompiler::Compile(_context, &oSource, v8::ScriptCompiler::kNoCompileOptions); + if (scriptCacheMB.IsEmpty()) + return script; - if (pCacheData) + pCacheData = v8::ScriptCompiler::CreateCodeCache(scriptCacheMB.ToLocalChecked()->GetUnboundScript()); + + // Create the cache file only once there is something to write into it. + // Creating it up-front left a zero-length .cache behind whenever the + // compile above failed, and the next run then took the Exists(Path) + // branch and handed that empty buffer to kConsumeCodeCache. + if (pCacheData) + { + NSFile::CFileBinary oFileTest; + if (oFileTest.CreateFileW(Path)) { - // save cache to file oFileTest.WriteFile(pCacheData->data, (DWORD)pCacheData->length); oFileTest.CloseFile(); } From 9007ae404592b114c93187eb779d64a74b1bb889 Mon Sep 17 00:00:00 2001 From: Christoph Schaefer Date: Thu, 27 Aug 2026 12:28:39 +0200 Subject: [PATCH 2/3] fix(doctrenderer): guard the snapshot compile as well Review of #138 (thanks @moodyjmz) pointed out that CJSContext::generateSnapshot() carries the same unguarded ToLocalChecked() this PR fixed in CCacheDataScript::Compile(), and compiles the very same GetAllScript() bundle via GenerateEditorSnapshot(). V8_SUPPORT_SNAPSHOTS is defined unconditionally (DesktopEditor/doctrenderer/CMakeLists.txt:236), so it is live code and the process would still abort on the same class of input. Not applied as the suggested early return, though: returning from inside that block leaves the scope without ever calling SetDefaultContext(), and destroys the SnapshotCreator without a blob having been created -- which its destructor expects. The compile result is carried in bCompiled instead, the creator's lifecycle is completed either way, and the snapshot file is simply not written when the script did not compile. Writing one built from a context the script never ran in would be worse than writing none, since the next start would consume it happily. Assisted-by: ClaudeCode:claude-opus-5 Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Christoph Schaefer --- .../doctrenderer/js_internal/v8/v8_base.cpp | 27 ++++++++++++++++--- 1 file changed, 23 insertions(+), 4 deletions(-) diff --git a/DesktopEditor/doctrenderer/js_internal/v8/v8_base.cpp b/DesktopEditor/doctrenderer/js_internal/v8/v8_base.cpp index d99a595632..b9adcfaa25 100644 --- a/DesktopEditor/doctrenderer/js_internal/v8/v8_base.cpp +++ b/DesktopEditor/doctrenderer/js_internal/v8/v8_base.cpp @@ -470,6 +470,7 @@ namespace NSJSBase { #ifdef V8_SUPPORT_SNAPSHOTS bool result = false; + bool bCompiled = false; // Snapshot creator should be in its own scope, because it handles entering, exiting and disposing the isolate v8::SnapshotCreator snapshotCreator; v8::Isolate* isolate = snapshotCreator.GetIsolate(); @@ -489,16 +490,34 @@ namespace NSJSBase // Compile v8::Local source = v8::String::NewFromUtf8(isolate, script.c_str()).ToLocalChecked(); - v8::Local script = v8::Script::Compile(context, source).ToLocalChecked(); - // Run - script->Run(context).IsEmpty(); + + // Guarded for the same reason as CCacheDataScript::Compile() above: + // this compiles the very same GetAllScript() bundle (via + // GenerateEditorSnapshot()), so a bundle the engine cannot parse + // would otherwise abort the process here through ToLocalChecked() + // instead of being reported. try_catch is already live above. + // + // Deliberately not an early return: SetDefaultContext() must still be + // called before CreateBlob(), and SnapshotCreator's destructor expects + // a blob to have been created, so the creator's lifecycle is completed + // either way. The failure is carried out in bCompiled, and the snapshot + // file is simply not written -- emitting one built from a context the + // script never ran in would be worse than emitting none, because it + // would be consumed happily on the next start. + v8::MaybeLocal scriptMB = v8::Script::Compile(context, source); + if (!scriptMB.IsEmpty()) + { + bCompiled = true; + // Run + scriptMB.ToLocalChecked()->Run(context).IsEmpty(); + } snapshotCreator.SetDefaultContext(context); } v8::StartupData data = snapshotCreator.CreateBlob(v8::SnapshotCreator::FunctionCodeHandling::kKeep); // Save snapshot to file NSFile::CFileBinary snapshotFile; - if (data.data && snapshotFile.CreateFile(snapshotPath)) + if (bCompiled && data.data && snapshotFile.CreateFile(snapshotPath)) { snapshotFile.WriteFile(data.data, (DWORD)data.raw_size); snapshotFile.CloseFile(); From 00eca2e77f59232ae2e3273f23136fe2c035e206 Mon Sep 17 00:00:00 2001 From: Christoph Schaefer Date: Thu, 27 Aug 2026 13:35:09 +0200 Subject: [PATCH 3/3] fix(doctrenderer): report snapshot compile failures instead of swallowing them Follow-up to the review of #138 (thanks @moodyjmz). The previous commit stopped generateSnapshot() aborting the process, but traded the crash for silence: CV8TryCatch's destructor is empty (v8_base.h:811), so a caught exception is discarded unless Check() is called, and nothing called it here. A bundle the engine could not parse produced no diagnostic at all and a quietly missing snapshot -- the opposite of this PR's stated goal. - v8_base.cpp: call try_catch.Check() after the compile/run block, while the Context::Scope is still alive. It prints the message, line and stack trace to stderr exactly as the cache path does via runScript(), and is a no-op when nothing was caught. It deliberately does not feed back into bCompiled. A runtime throw out of Run() did not stop the snapshot being written before this PR, and silently changing that could withhold snapshots that are fine today; only a compile failure suppresses the write. - doctrenderer.cpp: the caller discarded GenerateEditorSnapshot()'s bool, which was defensible while failure meant the process died anyway. Now that it survives, a failure means the editor runs without its snapshot, so log it. Assisted-by: ClaudeCode:claude-opus-5 Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Christoph Schaefer --- DesktopEditor/doctrenderer/doctrenderer.cpp | 7 ++++++- .../doctrenderer/js_internal/v8/v8_base.cpp | 14 ++++++++++++++ 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/DesktopEditor/doctrenderer/doctrenderer.cpp b/DesktopEditor/doctrenderer/doctrenderer.cpp index a005f89008..802c3144a4 100644 --- a/DesktopEditor/doctrenderer/doctrenderer.cpp +++ b/DesktopEditor/doctrenderer/doctrenderer.cpp @@ -1095,7 +1095,12 @@ namespace NSDoctRenderer for (std::vector::const_iterator i = editors.begin(); i != editors.end(); i++) { - NSDoctRenderer::GenerateEditorSnapshot(*i, m_pInternal); + // The return value used to be worth ignoring because a bundle the engine + // could not parse aborted the process outright. Now that it is reported + // and survived, a failure here means the editor silently runs without its + // snapshot -- slower, but easy to miss. Say so. + if (!NSDoctRenderer::GenerateEditorSnapshot(*i, m_pInternal)) + std::cerr << "doctrenderer: snapshot generation failed for editor type " << (int)(*i) << std::endl; } #endif } diff --git a/DesktopEditor/doctrenderer/js_internal/v8/v8_base.cpp b/DesktopEditor/doctrenderer/js_internal/v8/v8_base.cpp index b9adcfaa25..f6f98f261d 100644 --- a/DesktopEditor/doctrenderer/js_internal/v8/v8_base.cpp +++ b/DesktopEditor/doctrenderer/js_internal/v8/v8_base.cpp @@ -512,6 +512,20 @@ namespace NSJSBase scriptMB.ToLocalChecked()->Run(context).IsEmpty(); } + // Surface whatever was caught -- the compile failure above, or a throw + // out of Run(). CV8TryCatch's destructor does not check, so without this + // the pending exception is discarded and a bundle the engine cannot parse + // produces no diagnostic at all: silence in place of the abort removed + // above, which is not an improvement. Check() prints the message, line + // and stack to stderr, is a no-op when nothing was caught, and has to run + // while the Context::Scope is still alive. + // + // It deliberately does not feed back into bCompiled: a runtime throw out + // of Run() did not stop the snapshot being written before this change, + // and quietly changing that could withhold snapshots that are fine today. + // Only a compile failure suppresses the write. + try_catch.Check(); + snapshotCreator.SetDefaultContext(context); } v8::StartupData data = snapshotCreator.CreateBlob(v8::SnapshotCreator::FunctionCodeHandling::kKeep);