[Webkit-unassigned] [Bug 230602] [JSC] implement Shadow Realm

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=230602

--- Comment #12 from Yusuke Suzuki <ysuzuki at apple.com> ---
Comment on attachment 439380
  --> https://bugs.webkit.org/attachment.cgi?id=439380
Patch

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

> Source/JavaScriptCore/runtime/IndirectEvalExecutable.cpp:71
> +IndirectEvalExecutable* IndirectEvalExecutable::createSafe(JSGlobalObject* globalObject, const SourceCode& source, DerivedContextType derivedContextType, bool isArrowFunctionContext, EvalContextType evalContextType, NakedPtr<JSObject>& resultingError)

Let's rename it to create.

> Source/JavaScriptCore/runtime/IndirectEvalExecutable.cpp:79
> +IndirectEvalExecutable* IndirectEvalExecutable::create(JSGlobalObject* globalObject, const SourceCode& source, DerivedContextType derivedContextType, bool isArrowFunctionContext, EvalContextType evalContextType)

And let's rename it to tryCreate instead.

> Source/JavaScriptCore/runtime/ShadowRealmConstructor.h:38
> +    static constexpr unsigned StructureFlags = Base::StructureFlags | HasStaticPropertyTable;

Since we have no table, HasStaticPropertyTable is not necessary.

> Source/JavaScriptCore/runtime/ShadowRealmPrototype.cpp:69
> +    RELEASE_ASSERT(thisRealm);

Let's use ASSERT.

> Source/JavaScriptCore/runtime/ShadowRealmPrototype.cpp:93
> +    RELEASE_ASSERT(thisRealm);

Ditto.

> Source/JavaScriptCore/runtime/ShadowRealmPrototype.cpp:98
> +    String s = asString(evalArg)->value(globalObject);

let's make it `script` or `string` instead of `s` for readability.

> Source/JavaScriptCore/runtime/ShadowRealmPrototype.cpp:99
> +    RETURN_IF_EXCEPTION(scope, encodedJSValue());

Use `{ }` instead of `encodedJSValue()` in new code.

> Source/JavaScriptCore/runtime/ShadowRealmPrototype.cpp:111
> +    JSValue parsedObject;
> +    if (s.is8Bit()) {
> +        LiteralParser<LChar> preparser(globalObject, s.characters8(), s.length(), NonStrictJSON, nullptr);
> +        parsedObject = preparser.tryLiteralParse();
> +    } else {
> +        LiteralParser<UChar> preparser(globalObject, s.characters16(), s.length(), NonStrictJSON, nullptr);
> +        parsedObject = preparser.tryLiteralParse();
> +    }
> +    RETURN_IF_EXCEPTION(scope, encodedJSValue());
> +    if (parsedObject)
> +        return JSValue::encode(parsedObject);

I think this part is not necessary. It is done for JSON like object evaluation, but Realm.evaluate's usage would be largely different from that.

> Source/JavaScriptCore/runtime/ShadowRealmPrototype.cpp:131
> +    RETURN_IF_EXCEPTION(scope,
> +        throwVMError(globalObject, scope, createTypeError(globalObject, "Error encountered during evaluation"_s)));

Running something in RETURN_IF_EXCEPTION's second parameter is not the right thing.
Since we are getting an exception,

RETURN_IF_EXCEPTION(scope, { });

would be the right thing if we would like to propagate error from the evaluation.
If we would like to convert TypeError when some errors occur, then, the code should be,

if (UNLIKELY(scope.exception())) {
    scope.clearException();
    return throwVMError(globalObject, scope, createTypeError(globalObject, "Error encountered during evaluation"_s)));
}

> Source/JavaScriptCore/runtime/VM.cpp:372
> +    , shadowRealmSpace ISO_SUBSPACE_INIT(heap, cellHeapCellType.get(), ShadowRealmObject)

Let's make it dynamic IsoSubspace by using DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER_SLOW.

> Source/JavaScriptCore/runtime/VM.h:515
> +    IsoSubspace shadowRealmSpace;

Let's make it dynamic IsoSubspace by using DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20210928/4dd3491e/attachment.htm>