Fix accessing properties defined by a bound function#2299
Conversation
|
Let's see copilot's review, also each bug fic should contain also a test case proving the fix. |
There was a problem hiding this comment.
Pull request overview
Fixes a runtime crash when an accessor property’s getter is a bound function, which currently triggers an InvalidCastException during property access in ObjectInstance.UnwrapFromGetter.
Changes:
- Adds special-case handling for
Function.BindFunctioninObjectInstance.UnwrapFromGetterto avoid casting it toFunction.Function.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (getter is Function.BindFunction bindFunction) return bindFunction._engine.Call(bindFunction, thisObject); | ||
|
|
||
| var functionInstance = (Function.Function) getter; | ||
| return functionInstance._engine.Call(functionInstance, thisObject); |
There was a problem hiding this comment.
bindFunction._engine.Call(bindFunction, thisObject) resolves to Engine.Call(JsValue, params JsValue[] arguments), meaning thisObject is passed as a function argument and the getter is invoked with this=undefined. That changes observable behavior (e.g., arguments.length inside the getter becomes 1) and isn’t the correct way to invoke an accessor getter.
Call the overload that sets the receiver as this and passes no arguments (e.g., Call(callable, thisObject, Arguments.Empty) / Call((ICallable)callable, thisObject, Arguments.Empty, ...)).
| if (getter is Function.BindFunction bindFunction) return bindFunction._engine.Call(bindFunction, thisObject); | |
| var functionInstance = (Function.Function) getter; | |
| return functionInstance._engine.Call(functionInstance, thisObject); | |
| if (getter is Function.BindFunction bindFunction) return bindFunction._engine.Call(bindFunction, thisObject, Arguments.Empty); | |
| var functionInstance = (Function.Function) getter; | |
| return functionInstance._engine.Call(functionInstance, thisObject, Arguments.Empty); |
| if (getter is Function.BindFunction bindFunction) return bindFunction._engine.Call(bindFunction, thisObject); | ||
|
|
||
| var functionInstance = (Function.Function) getter; | ||
| return functionInstance._engine.Call(functionInstance, thisObject); |
There was a problem hiding this comment.
This accessor invocation path is still tightly coupled to specific concrete function types (BindFunction vs Function.Function) and will continue to throw InvalidCastException for any other callable getter that isn’t Function.Function. Elsewhere in this file (e.g., TryGetValue) getters are treated as ICallable.
Consider switching UnwrapFromGetter to use IsCallable/ICallable (and then invoking via Engine.Call) so all valid callable getter types are supported without adding more special-cases over time.
| if (getter is Function.BindFunction bindFunction) return bindFunction._engine.Call(bindFunction, thisObject); | |
| var functionInstance = (Function.Function) getter; | |
| return functionInstance._engine.Call(functionInstance, thisObject); | |
| if (!getter.IsCallable()) | |
| { | |
| return Undefined; | |
| } | |
| var callable = (ICallable) getter; | |
| return callable.Call(thisObject, Arguments.Empty); |
| if (getter is Function.BindFunction bindFunction) return bindFunction._engine.Call(bindFunction, thisObject); | ||
|
|
There was a problem hiding this comment.
This change fixes a specific regression scenario (accessor getter is a bound function), but the PR doesn’t add a regression test to ensure it stays fixed. Please add a test that defines an accessor via Object.defineProperty(..., { get: someFn.bind(...) }) and asserts the property access returns the expected value (and ideally that arguments.length inside the getter remains 0).
|
Created issue #2300 so we can process this more easily. |
|
A more robust fix was made in #2301 and now fixed in main. |
When an object property is defined as a getter, which in turn is a bound function, accessing the property results in
InvalidCastExceptioninObjectInstance.UnwrapFromGetter, becausegettervariable isJint.Native.Function.BindFunctioninstead ofJint.Native.Function.Function.Explicit test for
BindFunctionis not ideal and perhapsICallableshould be used, but I'm not sure how to properly pass it toEngine.Test code to reproduce the behavior:
Before fix: throws
InvalidCastExceptionAfter fix: prints out
42