1930258 - JS_DeepFreezeObject triggers environment crash when freezing function objects

Status: UNCONFIRMED

(Core :: JavaScript Engine, defect, P3)

Description

[Nicolas Auvray [:Itms]]

Attachment: Minimal reproduction example of the crash

Steps to reproduce:

We embed SpiderMonkey in 0 A.D. and we are experiencing a crash when using ESR 102 and ESR 115 (we haven't tested ESR 128 yet).

We often use JS_DeepFreezeObject to freeze objects containing nested data that must not be modified during a game or when modding the game. We expose that function to scripts.

I have managed to reduce our setup to a minimal example attached below, in which the reported crash can be reproduced.

Actual results:

When we use JS_DeepFreezeObject on a function object during the execution of a script, the next script to be executed (using JS::Evaluate) will crash with the following stack trace:

mozjs115-debug.dll!js::NearestEnclosingExtensibleLexicalEnvironment(JSObject * env) Line 21
	at E:\spidermonkey\mozjs-115.16.1\js\src\vm\EnvironmentObject-inl.h(21)
mozjs115-debug.dll!js::GlobalOrEvalDeclInstantiation(JSContext * cx, JS::Handle<JSObject *> envChain, JS::Handle<JSScript *> script, js::GCThingIndex lastFun) Line 3957
	at E:\spidermonkey\mozjs-115.16.1\js\src\vm\EnvironmentObject.cpp(3957)
mozjs115-debug.dll!js::Interpret(JSContext * cx, js::RunState & state) Line 3857
	at E:\spidermonkey\mozjs-115.16.1\js\src\vm\Interpreter.cpp(3857)
mozjs115-debug.dll!MaybeEnterInterpreterTrampoline(JSContext * cx, js::RunState & state) Line 400
	at E:\spidermonkey\mozjs-115.16.1\js\src\vm\Interpreter.cpp(400)
mozjs115-debug.dll!js::RunScript(JSContext * cx, js::RunState & state) Line 458
	at E:\spidermonkey\mozjs-115.16.1\js\src\vm\Interpreter.cpp(458)
mozjs115-debug.dll!js::ExecuteKernel(JSContext * cx, JS::Handle<JSScript *> script, JS::Handle<JSObject *> envChainArg, js::AbstractFramePtr evalInFrame, JS::MutableHandle<JS::Value> result) Line 845
	at E:\spidermonkey\mozjs-115.16.1\js\src\vm\Interpreter.cpp(845)
mozjs115-debug.dll!js::Execute(JSContext * cx, JS::Handle<JSScript *> script, JS::Handle<JSObject *> envChain, JS::MutableHandle<JS::Value> rval) Line 877
	at E:\spidermonkey\mozjs-115.16.1\js\src\vm\Interpreter.cpp(877)
mozjs115-debug.dll!EvaluateSourceBuffer<mozilla::Utf8Unit>(JSContext * cx, js::ScopeKind scopeKind, JS::Handle<JSObject *> env, const JS::ReadOnlyCompileOptions & optionsArg, JS::SourceText<mozilla::Utf8Unit> & srcBuf, JS::MutableHandle<JS::Value> rval) Line 556
	at E:\spidermonkey\mozjs-115.16.1\js\src\vm\CompilationAndEvaluation.cpp(556)
mozjs115-debug.dll!JS::Evaluate(JSContext * cx, const JS::ReadOnlyCompileOptions & options, JS::SourceText<mozilla::Utf8Unit> & srcBuf, JS::MutableHandle<JS::Value> rval) Line 564
	at E:\spidermonkey\mozjs-115.16.1\js\src\vm\CompilationAndEvaluation.cpp(564)
deepfreezecrash.exe!main(int argc, const char * * argv) Line 53
	at e:\spidermonkey\deepfreezecrash\src\test.cpp(53)

Expected results:

This crash does not happen:

• when using JS_FreezeObject (which should be equivalent in the minimal example, but does not cover our use case in general)
• when the object to freeze is not a function
• on the execution of the script itself, but when executing a second script

The crash should never happen, or it should trigger an assertion instead of poisoning the JS interpreter for later.

Comment 1

[Jan de Mooij [:jandem]]

Yes, JS_DeepFreezeObject is a footgun because it freezes all slot values including (internal values in) reserved slots. Maybe we should change it to ignore reserved slots but with that fixed it's still not a great API to use on arbitrary objects because it ignores JS object property semantics.

This API is called by XPConnect in StackScopedClone when deepFreeze: true is passed to Cu.cloneInto. That's only used by a few tests: https://searchfox.org/mozilla-central/search?q=deepFreeze&path=&case=false&regexp=false

Comment 2

[Matthew Gaudet (he/him) [:mgaudet]]

Kind of sounds like we need a new API (or at least to audit deepFreeze: true)