Bug 190137 – [JSC] Add a C++ callable overload of objectConstructorSeal

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

[Koby]

Hi,
In node-jsc, I needed to be able to seal an object from c++. Thus, I've added a objectConstructorSeal overload which receives a JSObject, and refactor the existing objectConstructorSeal to use it (similar to objectConstructorFreeze).

I'm attaching the patch.

[Koby]

Created attachment 351244 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 351244 [details]
Patch

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

> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:795
>      if (UNLIKELY(!success)) {
> -        throwTypeError(exec, scope, "Unable to prevent extension in Object.seal"_s);
> -        return encodedJSValue();
> +        return throwTypeError(exec, scope, "Unable to prevent extension in Object.seal"_s);
>      }

Remove `{` and `}`.

> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:803
> +    VM& vm = exec->vm();
> +    auto scope = DECLARE_THROW_SCOPE(vm);

In this case (expression which can cause an error is the last expression, and we do not have the other expressions which can cause an error), we don't need it.

> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:811
> +    JSObject* result = objectConstructorSeal(exec, asObject(obj));
> +    RETURN_IF_EXCEPTION(scope, encodedJSValue());
> +    return JSValue::encode(result);

In this case, we can just do

return JSValue::enccode(objectConstructorSeal(exec, asObject(obj)));

[Koby]

Will upload a new patch. 
Note that your last two comments are also relevant to objectConstructorFreeze which follows (and which I basically copied). Should modify it too and upload a patch (maybe in a separate bug)?

[Keith Miller]

Comment on attachment 351244 [details]
Patch

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

>> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:803
>> +    auto scope = DECLARE_THROW_SCOPE(vm);
> 
> In this case (expression which can cause an error is the last expression, and we do not have the other expressions which can cause an error), we don't need it.

I'm personally of the belief that we should have the scope anyway as the code can change in the future. It's good to be safe.

>> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:811
>> +    return JSValue::encode(result);
> 
> In this case, we can just do
> 
> return JSValue::enccode(objectConstructorSeal(exec, asObject(obj)));

If you decide to keep the scope, I would do:

scope.release();
return JSValue::enccode(objectConstructorSeal(exec, asObject(obj)));

Maybe we should have a macro for releasing then returning? We seem to do it a lot.

[Koby]

@Yusuke what do you think? should I remove the scope or leave it and use the new RELEASE_AND_RETURN?

[Yusuke Suzuki]

(In reply to Koby from comment #5)
> @Yusuke what do you think? should I remove the scope or leave it and use the
> new RELEASE_AND_RETURN?

Both are OK to me if you use RETURN_AND_RELEASE (which is already defined in runtime/ExceptionScope.h).

[Koby]

OK, i'll update the code.
Just one more thing that I've noticed and wanted to be sure about: In both objectConstructorSeal and objectConstructorFreeze, if setIntegrityLevel throws an exception an empty value is returned, while if it returns false, an exception is (thrown and) returned. Is this desirable? just making sure.

[Yusuke Suzuki]

(In reply to Koby from comment #7)
> OK, i'll update the code.
> Just one more thing that I've noticed and wanted to be sure about: In both
> objectConstructorSeal and objectConstructorFreeze, if setIntegrityLevel
> throws an exception an empty value is returned, while if it returns false,
> an exception is (thrown and) returned. Is this desirable? just making sure.

Except for some intentional cases (for optimization, see EXCEPTION_ASSERT code in JSC :) ), it is OK that the returned value of the function causing an exception is anything.

[Koby]

Created attachment 351507 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 351507 [details]
Patch

r=me

[Koby]

@Yusuke great, I wasn't sure :)
So just a couple of notes regarding the new patch:
- So now that I've removed the RETURN_IF_EXCEPTION, the javascript callable objectConstructorSeal might return the exception, instead of always returning an empty value (but in the previous comment @Yusuke wrote that it's OK).
- If approved, I would do the same change (RELEASE_AND_RETURN) for objectConstructorFreeze, which is exactly the same. I didn't want to add it to this patch because it's not directly related. What do you t think?

[WebKit Commit Bot]

Comment on attachment 351507 [details]
Patch

Clearing flags on attachment: 351507

Committed r236791: <https://trac.webkit.org/changeset/236791>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/44970666>