fix(doctrenderer): don't abort the process when a script fails to compile

[chrip]

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

[moodyjmz]

TL;DR: Fix is correct and safe — verified the caller graph, the Check()/Run() guard ordering, and CachedData ownership; no crash or leak introduced. One gap: CJSContext::generateSnapshot() (v8_base.cpp:492) has the identical unguarded .ToLocalChecked() pattern, compiles the same GetAllScript() bundle via GenerateEditorSnapshot(), and would still abort the process on the same class of bad-bundle input. V8_SUPPORT_SNAPSHOTS is unconditionally defined (CMakeLists.txt:236), so it's live code, not a dead config. A CV8TryCatch is already constructed one line above the compile (:476), so the guard is a two-line addition.

Full review

What's right

• v8_base.cpp:99-101: checks MaybeLocal::IsEmpty() before unwrapping, returns the default-empty script on failure — matches the three sibling compile sites already in the function.
• Traced every real caller reaching this path (only editors.cpp:209, gated on m_bIsUseCache). Its try_catch (CV8TryCatch) is constructed and live before the compile, so runScript's !exception->Check() guard correctly skips _script->Run() on the now-empty script — no null-deref introduced.
• Cache-file reordering (:109-117) fixes the zero-length .cache left behind on abort. Minor note: V8's CachedData sanity check would reject a 0-byte cache and recompile rather than crash again, so the original bug report slightly oversells this bullet — but it's still correct hygiene.
• pCacheData ownership (transferred to oSource2, freed by its dtor), the #else pre-V8-8.9 branch being left alone (confirmed V8_VERSION_89_PLUS is unconditionally defined for the CMake toolchain), and Path state on the early return — all checked, all clean.

What's missing

v8_base.cpp:492, CJSContext::generateSnapshot():

v8::Local<v8::Script> script = v8::Script::Compile(context, source).ToLocalChecked();

Same unguarded pattern, same abort. Reachable via GenerateEditorSnapshot() (editors.cpp:216-227), which calls this with the output of the same GetAllScript() bundle-builder that produced the broken sdkjs bundle in the original bug report (Euro-Office/sdkjs#80). If a future Terser/ICU-adjacent bundle bug recurs and snapshot generation hits it first, the process still aborts exactly as before — the PR body's claim that this "makes any future occurrence diagnosable rather than fatal" holds for the cache path but not for snapshot generation.

Minor, non-blocking

• :120-125: the second oSource2/kConsumeCodeCache compile is now unconditionally redundant — the first compile's Local<Script> is already valid and usable in _context. Pre-PR this was only redundant when CreateFileW succeeded; now it always double-compiles. Pre-existing pattern, not a correctness issue, not worth blocking on.
• Pre-existing, unrelated to this PR: runScript (:456-458) dereferences _script unguarded when exception is null — currently unreachable for the cache path since every caller passes a live try_catch, but a latent trap if that ever changes.

Cross-checked with an independent cold read against source (not just the diff); both passes converged on the caller-safety conclusion and independently landed on the generateSnapshot gap.

[moodyjmz]

Thanks for the fast follow-up — the SnapshotCreator lifecycle handling (bCompiled, unconditional SetDefaultContext/CreateBlob) is the right call; an early return there would've hit the destructor's blob-expectation. Good catch on that.

One thing left before this fully closes the loop, though: try_catch.Check() is never called in generateSnapshot().

CV8TryCatch::Check() (v8_base.h:816-853) is what actually reports the failure — it prints the syntax error, line number and stack trace to std::cerr. That's why the runScript/cache path works end-to-end: v8_base.cpp:453 calls exception->Check(), which both surfaces the diagnostic and gates Run(). In the new generateSnapshot() code, try_catch is constructed (:477) but Check() is never invoked anywhere in the function — on a compile failure you just set bCompiled = false and skip the file write. The pending exception is silently discarded when try_catch goes out of scope.

And the caller drops the ball too: doctrenderer.cpp:1098 is a bare NSDoctRenderer::GenerateEditorSnapshot(*i, m_pInternal); — the bool return is discarded, not even logged.

Net effect: a bad bundle now produces zero diagnostic output and a silently-missing snapshot, instead of the SIGTRAP abort. That's a real improvement (no more process death), but it's the opposite of the PR's stated goal — "the underlying SyntaxError never reported anywhere" is still true here, just without the crash to flag that something went wrong.

Suggested fix: call try_catch.Check() on the failure branch (before or after SetDefaultContext, doesn't matter — it just needs to run once while try_catch is still alive) so the error at least hits stderr, same as the cache path does.

[moodyjmz]

Verified 00eca2e against source — this closes it. try_catch.Check() runs unconditionally after the compile/run block, while context_scope is still alive, so a compile failure now prints message/line/stack to stderr exactly as the cache path does via runScript. Not feeding the result into bCompiled is the right call — Run()'s result was already discarded pre-PR, so a runtime throw never suppressed the snapshot write, and changing that now would be unrelated scope creep. doctrenderer.cpp:1098 logs per-editor-type failures instead of discarding the bool.

One thing worth a note for whoever eventually tests this rather than a blocker: CV8TryCatch::Check()'s Message()->GetLineNumber(...) / GetSourceLine(...).ToLocalChecked() (v8_base.h:820-823) is now load-bearing for both the cache and snapshot paths, and — since the triggering sdkjs bundle was fixed upstream — has never actually executed against a real failing compile. It should hold (V8 populates Message() for parse errors same as runtime ones), but nobody's run a deliberately malformed bundle through either path to confirm it doesn't itself blow up on ToLocalChecked().

No other findings. LGTM.

[moodyjmz]

Reviewed across three iterations (crash fix, snapshot-path guard, exception reporting) — verified each against source, not just commit messages. No open findings.