Bug 235382 – [JSC] move function wrapping logic to a new Function type

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Caitlin Potter (:caitp)]

Bug for creating an alternative implementation of the ShadowRealm wrapper function, based on JSBoundFunction.

[Caitlin Potter (:caitp)]

Created attachment 449517 [details]
Patch

This is just a starter patch, it has some things which could be better --- For the thunk, the rethrowing logic does not actually work, and I need some clarification on getting that set up. It might be good for allocation to occur inline, rather than in C operations. As noted in the changelog, at this time thhtere isn't anything added to DFG/FTL, but potentially there are opportunities there. I'm hoping I can get some feedback on the initial version and see if we actually prefer this to the JS builtin we've already got.

[Caitlin Potter (:caitp)]

Comment on attachment 449517 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=449517&action=review

> Source/JavaScriptCore/jit/ThunkGenerators.cpp:1494
> +    CCallHelpers::Jump noCode;

I think it's not observable if this is removed, so the thunk can be a bit smaller without this (though I don't believe it makes a big difference)

> Source/JavaScriptCore/jit/ThunkGenerators.cpp:1533
> +        jit.call(GPRInfo::nonArgGPR0, OperationPtrTag);

loop body is grown a bit because a lot of the wrapper logic is done inline, and only jumps to C to allocate a JSRemoteFunction or throw. I'm not sure if we want to do all the work in C, or do all the work inline here --- left as-is for now for simplicity.

T1 is manually saved, and the address of the callee is restored in T0 in each loop -- I'm sure it's fine to not restore the callee, which could potentially free up a register and avoid the need to save registers.

I might do that on the next change

> Source/JavaScriptCore/jit/ThunkGenerators.cpp:1575
> +    CCallHelpers::Jump targetFunctionException = jit.emitJumpIfException(vm);

So this is a real problem, because right now, the inner realm's exception handler is leaked to the caller realm. The 'targetFunctionException' label below needs to be registered as an exception handler location so that the leak can be avoided. I could use some guidance on how the handler locations are organized in JSC in order to do this.

> Source/JavaScriptCore/jit/ThunkGenerators.cpp:1594
> +    jit.call(GPRInfo::nonArgGPR0, OperationPtrTag);

Ditto, may want to do this inline (or at least inline the primitive cases, as is done above)

> Source/JavaScriptCore/jit/ThunkGenerators.cpp:1595
> +    exceptionChecks.append(jit.emitJumpIfException(vm));

I'm guessing there's no reason the C function can't do the unwind on its own and avoid this emitJumpIfException business?

> Source/JavaScriptCore/jit/ThunkGenerators.cpp:1605
> +    jit.call(GPRInfo::nonArgGPR0, OperationPtrTag);

the rethrow stub itself is kind of a mess and untested, since I can't get JS code to actually reach this point. It attempts to do what is already done in js, which is hopefully adequate.

> Source/JavaScriptCore/jsc.cpp:2089
> +JSC_DEFINE_HOST_FUNCTION(functionDollarIsRemoteFunction, (JSGlobalObject* globalObject, CallFrame* callFrame))

mainly added to help with debugging, but could be helpful in tests?

[Caitlin Potter (:caitp)]

I wouldn't pay attention to the EWS failures on mac yet, as the project files haven't been changed to add the new sources.

[Caitlin Potter (:caitp)]

Created attachment 449522 [details]
Patch

[Caitlin Potter (:caitp)]

Created attachment 449533 [details]
Patch

[Caitlin Potter (:caitp)]

Created attachment 449540 [details]
Patch

hopefully that fixed up the webkit xcode build

[Caitlin Potter (:caitp)]

Created attachment 449569 [details]
Patch

fixup ThrowScope assertions

[Yusuke Suzuki]

Comment on attachment 449569 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=449569&action=review

r=me with comments.

> Source/JavaScriptCore/builtins/ShadowRealmPrototype.js:37
>          delete wrapped['name'];
>          delete wrapped['length'];

Is it possible to remove them and integrate these changes into C++ JSRemoteFunction?

> Source/JavaScriptCore/jit/JITOperations.cpp:150
> +    JSValue result = getWrappedValue(globalObject, targetGlobalObject, JSValue::decode(encodedValue));
> +    RETURN_IF_EXCEPTION(scope, JSValue::encode(result));
> +
> +    scope.release();
> +    return JSValue::encode(result);

Let's use

RELEASE_AND_RETURN(scope, JSValue::encode(getWrappedValue(globalObject, targetGlobalObject, JSValue::decode(encodedValue))));

> Source/JavaScriptCore/jit/JITOperations.cpp:167
> +    JSValue result = getWrappedValue(globalObject, globalObject, JSValue::decode(encodedValue));
> +    RETURN_IF_EXCEPTION(scope, JSValue::encode(result));
> +    scope.release();
> +    return JSValue::encode(result);

Let's use

RELEASE_AND_RETURN(scope, JSValue::encode(getWrappedValue(globalObject, globalObject, JSValue::decode(encodedValue))));

> Source/JavaScriptCore/jit/JITOperations.cpp:183
> +    JSValue exceptionValue = scope.exception()->value();
> +    scope.clearException();

Let's return if it is terminated exception.
So,

Exception* exception = scope.exception();
if (UNLIKELY(vm.isTerminationException(exception))) {
    scope.release();
    return { };
}
JSValue exceptionValue = exception->value()

> Source/JavaScriptCore/jit/JITOperations.cpp:186
> +    scope.clearException();

Ditto.

Exception* toStringException = scope.exception();
if (ULIKELY(..._))
    ...

> Source/JavaScriptCore/jit/JITOperations.cpp:189
> +        throwTypeError(globalObject, scope, exceptionString);

You can use

return throwVMTypeError(...);

> Source/JavaScriptCore/jit/JITOperations.cpp:191
> +        throwTypeError(globalObject, scope);

Ditto.

> Source/JavaScriptCore/jit/JITOperations.cpp:193
> +    return encodedJSValue();

So, this line is not necessary.

> Source/JavaScriptCore/jit/JITOperations.h:98
> +    Jrf: JSRemoteFunction*

We do not need this since CCallHelpers can automatically handle them. So this is not necessary :)

> Source/JavaScriptCore/jit/JITOperations.h:151
> +using J_JITOperation_JrfJ = EncodedJSValue(JIT_OPERATION_ATTRIBUTES *)(JSRemoteFunction*, EncodedJSValue);

Ditto.

> Source/JavaScriptCore/jit/ThunkGenerators.cpp:1515
> +    // For each argument (order should not be observable):
> +    //     if the value is a Primitive, copy it into the new call frame arguments, otherwise
> +    //     perform wrapping logic. If the wrapping logic results in a new JSRemoteFunction,
> +    //     copy it into the new call frame's arguments, otherwise it must have thrown a TypeError.

In the current code, we are copying right-to-left.
Is it possible that operationGetWrappedValueForTarget's exception reveals this ordering?

> Source/JavaScriptCore/jit/ThunkGenerators.cpp:1517
> +    jit.sub32(CCallHelpers::TrustedImm32(1), GPRInfo::regT1);
> +    CCallHelpers::Jump done = jit.branchTest32(CCallHelpers::Zero, GPRInfo::regT1);

We can use branchSub32 here.

> Source/JavaScriptCore/jit/ThunkGenerators.cpp:1544
> +        jit.sub32(CCallHelpers::TrustedImm32(1), GPRInfo::regT1);
> +        jit.branchTest32(CCallHelpers::NonZero, GPRInfo::regT1).linkTo(loop, &jit);

We can use branchSub32 here.

> Source/JavaScriptCore/jit/ThunkGenerators.cpp:1563
> +#if ENABLE(ASSERT) && !CPU(ARM64E)

Use ASSERT_ENABLED

> Source/JavaScriptCore/jit/ThunkGenerators.cpp:1574
> +    // rethrow logic won't occur here. Need to set up an 

Looks like comment is trunced.

> Source/JavaScriptCore/jit/ThunkGenerators.cpp:1579
> +    JSValueRegs resultRegs(GPRInfo::regT0);

Let's use GPRInfo::returnValueGPR.

> Source/JavaScriptCore/jit/ThunkGenerators.cpp:1614
> +    targetFunctionException.link(&jit);
> +    jit.loadCell(CCallHelpers::addressFor(CallFrameSlot::callee), GPRInfo::regT2);
> +    jit.setupArguments<decltype(operationThrowRemoteFunctionException)>(GPRInfo::regT2);
> +    jit.prepareCallOperation(vm);
> +    jit.call(GPRInfo::nonArgGPR0, OperationPtrTag);
> +    // Fall through to jump to handler
> +
> +    exceptionChecks.link(&jit);
> +    jit.copyCalleeSavesToEntryFrameCalleeSavesBuffer(vm.topEntryFrame);
> +    jit.setupArguments<decltype(operationLookupExceptionHandler)>(CCallHelpers::TrustedImmPtr(&vm));
> +    jit.prepareCallOperation(vm);
> +    jit.move(CCallHelpers::TrustedImmPtr(tagCFunction<OperationPtrTag>(operationLookupExceptionHandler)), GPRInfo::nonArgGPR0);
> +    emitPointerValidation(jit, GPRInfo::nonArgGPR0, OperationPtrTag);
> +    jit.call(GPRInfo::nonArgGPR0, OperationPtrTag);

Can we have a test using this path?

> Source/JavaScriptCore/runtime/JSFunctionInlines.h:84
> +    return static_cast<bool>(jsDynamicCast<const JSRemoteFunction*>(globalObject()->vm(), this));

You can use,

return inherits<JSRemoteFunction>(vm)

> Source/JavaScriptCore/runtime/JSRemoteFunction.cpp:55
> +        return JSValue(JSRemoteFunction::create(vm, targetGlobalObject, target));

JSValue(...) is not necessary.

> Source/JavaScriptCore/runtime/JSRemoteFunction.cpp:66
> +    if (result.isEmpty())

We can use if (!result)

> Source/JavaScriptCore/runtime/JSRemoteFunction.cpp:76
> +    if (result.isEmpty())

Ditto.

> Source/JavaScriptCore/runtime/JSRemoteFunction.cpp:81
> +JSC_DEFINE_HOST_FUNCTION(remoteJSFunctionCall, (JSGlobalObject* globalObject, CallFrame* callFrame))

Let's rename it to remoteFunctionCallForJSFunction to make it more explicit.

> Source/JavaScriptCore/runtime/JSRemoteFunction.cpp:87
> +    JSFunction* target = remoteFunction->targetFunction();

Let's do,

JSFunction* target = jsCast<JSFunction*>(remoteFunction->targetFunction());

> Source/JavaScriptCore/runtime/JSRemoteFunction.cpp:123
> +JSC_DEFINE_HOST_FUNCTION(remoteFunctionCall, (JSGlobalObject* globalObject, CallFrame* callFrame))

Let's rename remoteFunctionCall to remoteFunctionCallGeneric to make it more explicit.

> Source/JavaScriptCore/runtime/JSRemoteFunction.h:56
> +    JSCell* target() { return m_target.get(); }

Let's rename it to targetFunction().

> Source/JavaScriptCore/runtime/JSRemoteFunction.h:57
> +    JSFunction* targetFunction() { return static_cast<JSFunction*>(getJSFunction(target())); }

Let's drop it.

> Source/JavaScriptCore/runtime/JSRemoteFunction.h:76
> +    WriteBarrier<JSCell> m_target;

Let's rename it to m_targetFunction to align it to JSBoundFunction.
And let's hold JSObject

[Caitlin Potter (:caitp)]

Created attachment 450037 [details]
Patch

Address Yusuke's comments. 32bit expected to still fail

[Caitlin Potter (:caitp)]

Comment on attachment 449569 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=449569&action=review

>> Source/JavaScriptCore/jit/ThunkGenerators.cpp:1574
>> +    // rethrow logic won't occur here. Need to set up an 
> 
> Looks like comment is trunced.

Oops. I mentioned this in the slack thread -- I think we need a way to get to this point from unwindGeneric(), because otherwise, exceptions thrown in a JS call will reach a different handler in unwindGeneric, similar to the problem on 32bit.

I've fixed the comment:

// FIXME: if an exception is thrown within a JS call, we'll end up in an outer exception handler and the
// rethrow logic won't occur here. We will need to add some way to get here from unwindGeneric().

>> Source/JavaScriptCore/jit/ThunkGenerators.cpp:1614
>> +    jit.call(GPRInfo::nonArgGPR0, OperationPtrTag);
> 
> Can we have a test using this path?

this is reached by shadow-realm-import-value.js at different points (for example)

Adding a jit.print() this line gives me:

```
Tools/Scripts/run-jsc --jsc-only JSTests/stress/shadow-realm-import-value.js --use
ShadowRealm=1 --useDollarVM=1 --useLLInt=0
Running 1 time(s): DYLD_FRAMEWORK_PATH=/home/caitp/src/WebKit/WebKitBuild/Release/bin /home/caitp/src/WebKit/WebKitBuild/Release/bin/jsc --useDollarVM=1 JSTests/stress/shadow-realm-import-value.js --useShadowRealm=1 --useDollarVM=1 --useLLInt=0
remoteFunctionCallGenerator exceptionChecks
remoteFunctionCallGenerator exceptionChecks
remoteFunctionCallGenerator exceptionChecks
```

Unfortunately the targetFunctionException branch is not reachable, AFAICT (hence the FIXME)

[Yusuke Suzuki]

Comment on attachment 450037 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=450037&action=review

> Source/JavaScriptCore/builtins/ShadowRealmPrototype.js:-60
> -        delete wrapped['name'];
> -        delete wrapped['length'];

IIRC, JSFunction's name and length properties are specially handled (see FunctionRareData etc.).
So, if we set setHasReifiedName / setHasReifiedLength appropriately (while not defining them in Structure), and if we configure JSFunction::hasReifiedName / JSFunction::hasReifiedLength to return true for this type of function (to avoid always allocating FunctionRareData),
then, I think we can achieve the goal.

> Source/JavaScriptCore/runtime/JSFunctionInlines.h:109
> +    JSFunction* function = jsCast<JSFunction*>(getJSFunction(value));

jsCast is expecting that is it is always JSFunction*. Use `jsDynamicCast` instead.

> Source/JavaScriptCore/runtime/JSRemoteFunction.cpp:71
> +static inline JSValue wraprReturnValue(JSGlobalObject* globalObject, JSGlobalObject* targetGlobalObject, JSValue value)

wraprReturnValue => wrapReturnValue

> Source/JavaScriptCore/runtime/JSRemoteFunction.cpp:98
> +        return encodedJSValue();

Use `{ }` in new code.

> Source/JavaScriptCore/runtime/JSRemoteFunction.cpp:114
> +        return encodedJSValue();

Use `{ }` in new code.

> Source/JavaScriptCore/runtime/JSRemoteFunction.cpp:118
> +    auto wrappedResult = wraprReturnValue(globalObject, globalObject, result);
> +    RELEASE_AND_RETURN(scope, JSValue::encode(wrappedResult));

Let's release the scope before calling wraprReturnValue, so, change it to

RELEASE_AND_RETURN(scope, JSValue::encode(wraprReturnValue(globalObject, globalObject, result)))

> Source/JavaScriptCore/runtime/JSRemoteFunction.cpp:137
> +        return encodedJSValue();

Use `{ }` in new code.

> Source/JavaScriptCore/runtime/JSRemoteFunction.cpp:148
> +        return encodedJSValue();

Use `{ }` in new code.

> Source/JavaScriptCore/runtime/JSRemoteFunction.cpp:152
> +    auto wrappedResult = wrapValue(targetGlobalObject, globalObject, result);
> +    RELEASE_AND_RETURN(scope, JSValue::encode(wrappedResult));

Ditto. Change it to,

RELEASE_AND_RETURN(scope, JSValue::encode(wrapValue(targetGlobalObject, globalObject, result)));

to release the scope before calling wrapValue

> Source/JavaScriptCore/runtime/JSRemoteFunction.cpp:179
> +    auto result = JSRemoteFunction::create(vm, destinationGlobalObject, targetCallable);
> +    RELEASE_AND_RETURN(scope, JSValue::encode(result));

Ditto.

[Caitlin Potter (:caitp)]

Created attachment 450055 [details]
Patch

Address the latest comments

[Yusuke Suzuki]

Comment on attachment 450055 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=450055&action=review

Clearing r? since it is not completed. You can upload a WIP patch without r? by using `--no-review` :)

> Source/JavaScriptCore/builtins/ShadowRealmPrototype.js:-60
> -        delete wrapped['name'];
> -        delete wrapped['length'];

Ditto to the previous comment.

> Source/JavaScriptCore/runtime/JSFunctionInlines.h:82
> +inline bool JSFunction::isRemoteFunction() const

Let's take VM& as a parameter.

> Source/JavaScriptCore/runtime/JSFunctionInlines.h:107
> +inline bool isRemoteFunction(JSValue value)

Let's take VM&.

> Source/JavaScriptCore/runtime/JSFunctionInlines.h:112
> +    JSFunction* function = jsCast<JSFunction*>(getJSFunction(value));
> +    if (!function)
> +        return false;
> +    return function->isRemoteFunction();

Let's write as follows. I don't think we need getJSFunction here. And let's take VM&.

inline bool isRemoteFunction(VM& vm, JSValue value)
{
    if (auto* function = jsDynamicCast<JSFunction*>(vm, value))
        return function->isRemoteFunction(vm);
    return false;
}

[Radar WebKit Bug Importer]

<rdar://problem/88093995>

[Caitlin Potter (:caitp)]

Comment on attachment 450037 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=450037&action=review

>> Source/JavaScriptCore/builtins/ShadowRealmPrototype.js:-60
>> -        delete wrapped['length'];
> 
> IIRC, JSFunction's name and length properties are specially handled (see FunctionRareData etc.).
> So, if we set setHasReifiedName / setHasReifiedLength appropriately (while not defining them in Structure), and if we configure JSFunction::hasReifiedName / JSFunction::hasReifiedLength to return true for this type of function (to avoid always allocating FunctionRareData),
> then, I think we can achieve the goal.

hmm, I did test this locally --- Object.hasOwn(wrapperFunction, 'name'/'length') returned false -- but i'll take another look I guess

[Yusuke Suzuki]

Comment on attachment 450037 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=450037&action=review

>>> Source/JavaScriptCore/builtins/ShadowRealmPrototype.js:-60
>>> -        delete wrapped['length'];
>> 
>> IIRC, JSFunction's name and length properties are specially handled (see FunctionRareData etc.).
>> So, if we set setHasReifiedName / setHasReifiedLength appropriately (while not defining them in Structure), and if we configure JSFunction::hasReifiedName / JSFunction::hasReifiedLength to return true for this type of function (to avoid always allocating FunctionRareData),
>> then, I think we can achieve the goal.
> 
> hmm, I did test this locally --- Object.hasOwn(wrapperFunction, 'name'/'length') returned false -- but i'll take another look I guess

Ah, probably it works. Can you review the existing code to ensure this works?

[Caitlin Potter (:caitp)]

Created attachment 450320 [details]
Patch

Handle rethrowing logic in Interpreter::unwind() --- the way the handler's callframe is determined is potentially broken, I'll write some tests for it on monday

[Yusuke Suzuki]

Comment on attachment 450320 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=450320&action=review

Nice direction!

> Source/JavaScriptCore/interpreter/Interpreter.cpp:690
> +        exceptionString = static_cast<ErrorInstance*>(exceptionValue.asCell())->sanitizedMessageString(globalObject);

Let's change `toWTFString` code in sanitizedMessageString as I suggested to avoid user-observable behavior.
Currently, if user puts `{ toString() { user-custom-code } }` to message property, then it becomes observable.

[Caitlin Potter (:caitp)]

Comment on attachment 450320 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=450320&action=review

>> Source/JavaScriptCore/interpreter/Interpreter.cpp:690
>> +        exceptionString = static_cast<ErrorInstance*>(exceptionValue.asCell())->sanitizedMessageString(globalObject);
> 
> Let's change `toWTFString` code in sanitizedMessageString as I suggested to avoid user-observable behavior.
> Currently, if user puts `{ toString() { user-custom-code } }` to message property, then it becomes observable.

On monday I'll change it to bail if messageValue.isPrimitive() is false

[Caitlin Potter (:caitp)]

Created attachment 450442 [details]
Patch

Handle rethrowing logic in Interpreter::unwind() --- the way the handler's callframe is determined is potentially broken, I'll write some tests for it on monday

[Caitlin Potter (:caitp)]

(In reply to Caitlin Potter (:caitp) from comment #20)
> Created attachment 450442 [details]
> Patch
> 
> Handle rethrowing logic in Interpreter::unwind() --- the way the handler's
> callframe is determined is potentially broken, I'll write some tests for it
> on monday

s/that comment/"""
Add CopyNameAndLength logic, update tests, and fix up ErrorInstance::sanitizedMessageString to only attempt to stringify a primitive value, avoiding potential observable side effects.
"""

[Caitlin Potter (:caitp)]

Comment on attachment 450442 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=450442&action=review

> Source/JavaScriptCore/jit/JITOperations.cpp:163
> +JSC_DEFINE_JIT_OPERATION(operationThrowRemoteFunctionException, EncodedJSValue, (JSRemoteFunction* callee))

TODO: delete this, since it isn't used

> Source/JavaScriptCore/jit/JITOperations.h:161
> +JSC_DECLARE_JIT_OPERATION(operationThrowRemoteFunctionException, EncodedJSValue, (JSRemoteFunction*));

ditto

> Source/JavaScriptCore/runtime/JSFunction.cpp:630
> +    if ((isHostOrBuiltinFunction() && !this->inherits<JSBoundFunction>(vm)))

TODO: unchange this

> Source/JavaScriptCore/runtime/JSFunction.cpp:706
> +        unsigned initialAttributes = PropertyAttribute::DontEnum | PropertyAttribute::ReadOnly;

The proposal text today says we should have a "wrapped " prefix, but per https://github.com/tc39/proposal-shadowrealm/pull/348 I haven't added it to this change.

> Source/JavaScriptCore/runtime/JSRemoteFunction.cpp:212
> +        m_length = std::max(targetLengthAsInt, 0.0);

Does not quite match the steps in the proposal as of Jan 31, performing the toIntegerOrInfinity even if targetLength is already a Number. This is because SetFunctionLength in the spec expects a non-negative integer or Infinity. AFAICT, The spec as-written today would be perfectly happy with a length of -57, or 3.14, but this doesn't really make any sense and I don't like that it's different from other cases.

The proposal text is copied from the existing Function.bind text, which is presumably written the way it is for interop, however as far as I can tell, the most popular browser does not implement it the way it's written. A simple test case:

```
function f() {}
Object.defineProperty(f, "length", { value: { valueOf() { return 3.14; } } });
let bound = f.bind();

f.length === 0; // I would expect f.length to be 3 per ToIntegerOrInfinity()
```

so the behaviour I've implemented makes more sense to me, and is more consistent --- but I think the thing to do is match what we're doing currently for bound functions, and that'll be the end of it.

[Caitlin Potter (:caitp)]

Comment on attachment 450442 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=450442&action=review

>> Source/JavaScriptCore/runtime/JSRemoteFunction.cpp:212
>> +        m_length = std::max(targetLengthAsInt, 0.0);
> 
> Does not quite match the steps in the proposal as of Jan 31, performing the toIntegerOrInfinity even if targetLength is already a Number. This is because SetFunctionLength in the spec expects a non-negative integer or Infinity. AFAICT, The spec as-written today would be perfectly happy with a length of -57, or 3.14, but this doesn't really make any sense and I don't like that it's different from other cases.
> 
> The proposal text is copied from the existing Function.bind text, which is presumably written the way it is for interop, however as far as I can tell, the most popular browser does not implement it the way it's written. A simple test case:
> 
> ```
> function f() {}
> Object.defineProperty(f, "length", { value: { valueOf() { return 3.14; } } });
> let bound = f.bind();
> 
> f.length === 0; // I would expect f.length to be 3 per ToIntegerOrInfinity()
> ```
> 
> so the behaviour I've implemented makes more sense to me, and is more consistent --- but I think the thing to do is match what we're doing currently for bound functions, and that'll be the end of it.

I may have been misreading the spec a bit. Going to change this to match what we already do, which should be interoperable.

[Caitlin Potter (:caitp)]

Created attachment 450467 [details]
Patch

rename JSRemoteFunction::create() to tryCreate() to hint that it may fail, delete unused code, and try to match Function.bind() function length logic better

[Caitlin Potter (:caitp)]

Created attachment 450922 [details]
patch-for-review

patch-for-review

[Yusuke Suzuki]

Comment on attachment 450922 [details]
patch-for-review

View in context: https://bugs.webkit.org/attachment.cgi?id=450922&action=review

r=me, can you grep source with `<JSBoundFunction` to ensure that these places are also considered for JSRemoteFunction too?

> Source/JavaScriptCore/interpreter/Interpreter.cpp:699
> +
> +    auto scope = DECLARE_THROW_SCOPE(vm);
> +    ASSERT(exception);
> +    ASSERT(!vm.isTerminationException(exception));
> +
> +    JSGlobalObject* globalObject = handlerCallFrame->jsCallee()->globalObject();
> +    JSValue exceptionValue = exception->value();
> +    scope.clearException();
> +
> +    // Avoid user-observable ToString()
> +    String exceptionString;
> +    if (exceptionValue.isPrimitive())
> +        exceptionString = exceptionValue.toWTFString(globalObject);
> +    else if (exceptionValue.asCell()->inherits<ErrorInstance>(vm))
> +        exceptionString = static_cast<ErrorInstance*>(exceptionValue.asCell())->sanitizedMessageString(globalObject);
> +
> +    ASSERT(!scope.exception()); // We must not have entered JS at this point
> +
> +    if (exceptionString.length()) {
> +        throwVMTypeError(globalObject, scope, exceptionString);
> +        return;
> +    }
> +
> +    throwVMTypeError(globalObject, scope);

Let's use DeferTermination in this scope (before DECLARE_THROW_SCOPE).
If you create DECLARE_THROW_SCOPE, then it is possible that you will get termination exception (since it can be set from the concurrent thread. Main usage of that is terminating a worker thread's JS VM).
DeferTermination can defer the handling of termination exception. I think this is good. Since, if we get termination exception here (since sanitizedMessageString uses DECLARE_THROW_SCOPE), we need to re-unwind the stack since termination exception must not be caught by user's handler, which complicate the code too much, while it does not offer much good thing (DeferTermination just defers it. Next time we use DECLARE_THROW_SCOPE etc. then we get termination exception so it will be super quickly handled).

> Source/JavaScriptCore/runtime/ErrorInstance.cpp:166
> +    if (!messageValue || !messageValue.isPrimitive())

Let's put this check too on sanitizedNameString

> Source/JavaScriptCore/runtime/JSFunction.cpp:630
> +    if ((isHostOrBuiltinFunction() && !this->inherits<JSBoundFunction>(vm)))

It looks like it is adding an unnecessary parentheses. Should we need to add JSRemoteFunction handling here?
Anyway, can we remove this parentheses since it is not necessary?

> Source/JavaScriptCore/runtime/JSFunctionInlines.h:28
>  #include "FunctionExecutable.h"

In this file, do we need to handle asStringConcurrently for JSRemoteFunction?

> Source/JavaScriptCore/runtime/JSFunctionInlines.h:111
> +    if (auto* function = jsDynamicCast<JSFunction*>(vm, value))
> +        return function->isRemoteFunction(vm);
> +    return false;

Ah, sorry. You can just do,

return value.inherits<JSRemoteFunction>(vm)

and that's enough.

> Source/JavaScriptCore/runtime/JSRemoteFunction.h:45
> +    typedef JSFunction Base;

Use using for the new code.

[Caitlin Potter (:caitp)]

Created attachment 451283 [details]
Patch for landing

[Caitlin Potter (:caitp)]

Comment on attachment 450922 [details]
patch-for-review

View in context: https://bugs.webkit.org/attachment.cgi?id=450922&action=review

>> Source/JavaScriptCore/interpreter/Interpreter.cpp:699
>> +    throwVMTypeError(globalObject, scope);
> 
> Let's use DeferTermination in this scope (before DECLARE_THROW_SCOPE).
> If you create DECLARE_THROW_SCOPE, then it is possible that you will get termination exception (since it can be set from the concurrent thread. Main usage of that is terminating a worker thread's JS VM).
> DeferTermination can defer the handling of termination exception. I think this is good. Since, if we get termination exception here (since sanitizedMessageString uses DECLARE_THROW_SCOPE), we need to re-unwind the stack since termination exception must not be caught by user's handler, which complicate the code too much, while it does not offer much good thing (DeferTermination just defers it. Next time we use DECLARE_THROW_SCOPE etc. then we get termination exception so it will be super quickly handled).

I've added this. Are the fuzzers able to find issues like this?

>> Source/JavaScriptCore/runtime/JSFunction.cpp:630
>> +    if ((isHostOrBuiltinFunction() && !this->inherits<JSBoundFunction>(vm)))
> 
> It looks like it is adding an unnecessary parentheses. Should we need to add JSRemoteFunction handling here?
> Anyway, can we remove this parentheses since it is not necessary?

yep fixed

>> Source/JavaScriptCore/runtime/JSFunctionInlines.h:28
>>  #include "FunctionExecutable.h"
> 
> In this file, do we need to handle asStringConcurrently for JSRemoteFunction?

I think I've matched the JSBoundFunction behaviour in the latest patch (return nullptr from asStringConcurrently, and use similar logic in JSFunction::toString() -- Hopefully that's fine.

[EWS]

ChangeLog entry in JSTests/ChangeLog contains OOPS!.

[Caitlin Potter (:caitp)]

Created attachment 451285 [details]
Patch for landing

[Caitlin Potter (:caitp)]

Created attachment 451286 [details]
Patch for landing

[Yusuke Suzuki]

Comment on attachment 450922 [details]
patch-for-review

View in context: https://bugs.webkit.org/attachment.cgi?id=450922&action=review

> Source/JavaScriptCore/interpreter/Interpreter.cpp:692
> +    ASSERT(!scope.exception()); // We must not have entered JS at this point

scope creation can potentially allow throwing a termination exception.
So, it is possible that we have termination exception here.
Let's keep it if it is.

>>> Source/JavaScriptCore/interpreter/Interpreter.cpp:699
>>> +    throwVMTypeError(globalObject, scope);
>> 
>> Let's use DeferTermination in this scope (before DECLARE_THROW_SCOPE).
>> If you create DECLARE_THROW_SCOPE, then it is possible that you will get termination exception (since it can be set from the concurrent thread. Main usage of that is terminating a worker thread's JS VM).
>> DeferTermination can defer the handling of termination exception. I think this is good. Since, if we get termination exception here (since sanitizedMessageString uses DECLARE_THROW_SCOPE), we need to re-unwind the stack since termination exception must not be caught by user's handler, which complicate the code too much, while it does not offer much good thing (DeferTermination just defers it. Next time we use DECLARE_THROW_SCOPE etc. then we get termination exception so it will be super quickly handled).
> 
> I've added this. Are the fuzzers able to find issues like this?

Yes, our fuzzer is attaching --watchdog=xxx sometimes, which causes termination exception in JSC shell.

[EWS]

Committed r289417 (246980@main): <https://commits.webkit.org/246980@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 451286 [details].