[webkit-reviews] review granted: [Bug 235382] [JSC] move function wrapping logic to a new Function type : [Attachment 450922] patch-for-review

[bugzilla-daemon at webkit.org]

Yusuke Suzuki <ysuzuki at apple.com> has granted Caitlin Potter (:caitp)
<caitp at igalia.com>'s request for review:
Bug 235382: [JSC] move function wrapping logic to a new Function type
https://bugs.webkit.org/show_bug.cgi?id=235382

Attachment 450922: patch-for-review

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




--- Comment #26 from Yusuke Suzuki <ysuzuki at apple.com> ---
Comment on attachment 450922
  --> https://bugs.webkit.org/attachment.cgi?id=450922
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(gl
obalObject);
> +
> +    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.