definePropertyOnReceiver should check if receiver canPerformFastPutInline

[Tadeu Zagallo]

...

[Tadeu Zagallo]

Created attachment 433517 [details]
Patch

[Tadeu Zagallo]

Created attachment 433523 [details]
Patch

[Tadeu Zagallo]

Created attachment 433526 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 433526 [details]
Patch

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

Add some comments :)

> Source/JavaScriptCore/runtime/JSObject.cpp:934
> +            RELEASE_ASSERT(!(entry->value->attributes() & PropertyAttribute::CustomValue));

This function is super hot. Please do not use RELEASE_ASSERT.

> Source/JavaScriptCore/runtime/JSObjectInlines.h:342
> +                RELEASE_ASSERT(!(currentAttributes & PropertyAttribute::AccessorOrCustomAccessorOrValue));

This function is super hot. Please do not use RELEASE_ASSERT.

> Source/JavaScriptCore/runtime/JSObjectInlines.h:402
> +            RELEASE_ASSERT(!(currentAttributes & PropertyAttribute::AccessorOrCustomAccessorOrValue));

Ditto.

[Radar WebKit Bug Importer]

<rdar://problem/80910723>

[Tadeu Zagallo]

Created attachment 434022 [details]
WIP

[Tadeu Zagallo]

Created attachment 434041 [details]
Patch

[Alexey Shvayka]

Comment on attachment 434041 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSObject.cpp:881
> +        if (slot.attributes() & PropertyAttribute::CustomValue) {

I think we can avoid adding this.
RegExpObject::defineOwnProperty() will take care of "lastIndex". WebIDL's `window.%Constructor%` and `window.%Constructor%.prototype.constructor` don't have a setter.
As for RegExpConstructor, its properties will be converted very soon (please follow https://bugs.webkit.org/show_bug.cgi?id=220233).

> Source/JavaScriptCore/runtime/JSObject.cpp:914
> +    if (!receiver->canPerformFastPutInline(vm, propertyName))

We can make this way more targeted to underline the CustomValue issue an because Accessor / CustomAccessor are handled correctly by putDirectInternal(). Like:
    if (!receiver->structure(vm)->hasCustomGetterSetterProperties()) {
        unsigned attributes;
        if (receiver->getDirectOffset(vm, propertyName, attributes) != invalidOffset && (attributes & PropertyAttribute::CustomValue))
            return definePropertyOnReceiverSlow(globalObject, propertyName, value, receiver, slot.isStrictMode());
    }

The snippet can be improved to avoid structure lookups.

> Source/JavaScriptCore/runtime/JSObject.cpp:928
> +    ASSERT(this->canPerformFastPutInline(vm, propertyName));

This won't hold if you apply previous comment.

> Source/JavaScriptCore/runtime/JSObject.cpp:-931
> -            putDirect(vm, propertyName, value, attributesForStructure(entry->value->attributes()) & ~PropertyAttribute::CustomValue, slot);

This won't be needed if you apply the comment before previous.

> Source/JavaScriptCore/runtime/JSObjectInlines.h:-285
> -    // FIXME: For a failure due to non-extensible structure, the error message is misleading.

There are quite a few FIXMEs like this; maybe we should tackle all of them in separate change to keep this (very tricky) patch simpler.

> JSTests/stress/reflect-set-custom-value.js:1
> +({...RegExp});

This test is very tricky.
Once https://bugs.webkit.org/show_bug.cgi?id=220233 is fixed, this test won't be testing anything useful.
Can we make another test utilizing JSDollarVM's StaticCustomValue and friends instead?
Maybe we could even simplify how we reach the state that currently causes a crash.

[Alexey Shvayka]

> RegExpObject::defineOwnProperty() will take care of "lastIndex".

Or maybe it won't because "lastIndex" is not on structure. Hmm...

[Alexey Shvayka]

definePropertyOnReceiver() performs [[DefineOwnProperty]], and semantically it should not invoke canPerformFastPutInline() or putInlineSlow(), which are for [[Set]].

[Alexey Shvayka]

(In reply to Alexey Shvayka from comment #9)
> > RegExpObject::defineOwnProperty() will take care of "lastIndex".
> 
> Or maybe it won't because "lastIndex" is not on structure. Hmm...

I think we are good for "lastIndex". For Reflect.set,

    if (slot.isTaintedByOpaqueObject() || slot.context() == PutPropertySlot::ReflectSet) {
        if (receiver->methodTable(vm)->defineOwnProperty != JSObject::defineOwnProperty)
            return definePropertyOnReceiverSlow(globalObject, propertyName, value, receiver, slot.isStrictMode());
    }

path should be taken. For regular set, RegExpObject::put() will preserve correctness.

[Alexey Shvayka]

Comment on attachment 434041 [details]
Patch

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

>> Source/JavaScriptCore/runtime/JSObject.cpp:881
>> +        if (slot.attributes() & PropertyAttribute::CustomValue) {
> 
> I think we can avoid adding this.
> RegExpObject::defineOwnProperty() will take care of "lastIndex". WebIDL's `window.%Constructor%` and `window.%Constructor%.prototype.constructor` don't have a setter.
> As for RegExpConstructor, its properties will be converted very soon (please follow https://bugs.webkit.org/show_bug.cgi?id=220233).

Removing this may require adjusting some Reflect.set(RegExp, ...) tests, but this is very doable and we aren't currently aligned with other engines either way.

[Tadeu Zagallo]

Comment on attachment 434041 [details]
Patch

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

>>> Source/JavaScriptCore/runtime/JSObject.cpp:881
>>> +        if (slot.attributes() & PropertyAttribute::CustomValue) {
>> 
>> I think we can avoid adding this.
>> RegExpObject::defineOwnProperty() will take care of "lastIndex". WebIDL's `window.%Constructor%` and `window.%Constructor%.prototype.constructor` don't have a setter.
>> As for RegExpConstructor, its properties will be converted very soon (please follow https://bugs.webkit.org/show_bug.cgi?id=220233).
> 
> Removing this may require adjusting some Reflect.set(RegExp, ...) tests, but this is very doable and we aren't currently aligned with other engines either way.

It's a simple condition, so we can just remove this when the other patch lands? Currently this will cause a crash, and I don't think we want to keep it in its current state on trunk. I can add a FIXME if that helps.

>> Source/JavaScriptCore/runtime/JSObject.cpp:914
>> +    if (!receiver->canPerformFastPutInline(vm, propertyName))
> 
> We can make this way more targeted to underline the CustomValue issue an because Accessor / CustomAccessor are handled correctly by putDirectInternal(). Like:
>     if (!receiver->structure(vm)->hasCustomGetterSetterProperties()) {
>         unsigned attributes;
>         if (receiver->getDirectOffset(vm, propertyName, attributes) != invalidOffset && (attributes & PropertyAttribute::CustomValue))
>             return definePropertyOnReceiverSlow(globalObject, propertyName, value, receiver, slot.isStrictMode());
>     }
> 
> The snippet can be improved to avoid structure lookups.

Sounds good, I'll give it a try.

>> Source/JavaScriptCore/runtime/JSObject.cpp:-931
>> -            putDirect(vm, propertyName, value, attributesForStructure(entry->value->attributes()) & ~PropertyAttribute::CustomValue, slot);
> 
> This won't be needed if you apply the comment before previous.

I think it's still relevant. We never want to end up here with a custom value.

>> Source/JavaScriptCore/runtime/JSObjectInlines.h:-285
>> -    // FIXME: For a failure due to non-extensible structure, the error message is misleading.
> 
> There are quite a few FIXMEs like this; maybe we should tackle all of them in separate change to keep this (very tricky) patch simpler.

I see... these should have had a bug to make it easier to track them. If I don't fix this particular instance, it breaks a couple tests, but I guess I can skip the tests in the mean time.

>> JSTests/stress/reflect-set-custom-value.js:1
>> +({...RegExp});
> 
> This test is very tricky.
> Once https://bugs.webkit.org/show_bug.cgi?id=220233 is fixed, this test won't be testing anything useful.
> Can we make another test utilizing JSDollarVM's StaticCustomValue and friends instead?
> Maybe we could even simplify how we reach the state that currently causes a crash.

Sounds good, I'll write a new test

[Alexey Shvayka]

(In reply to Tadeu Zagallo from comment #13)
> Comment on attachment 434041 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=434041&action=review
> 
> >>> Source/JavaScriptCore/runtime/JSObject.cpp:881
> >>> +        if (slot.attributes() & PropertyAttribute::CustomValue) {
> >> 
> >> I think we can avoid adding this.
> >> RegExpObject::defineOwnProperty() will take care of "lastIndex". WebIDL's `window.%Constructor%` and `window.%Constructor%.prototype.constructor` don't have a setter.
> >> As for RegExpConstructor, its properties will be converted very soon (please follow https://bugs.webkit.org/show_bug.cgi?id=220233).
> > 
> > Removing this may require adjusting some Reflect.set(RegExp, ...) tests, but this is very doable and we aren't currently aligned with other engines either way.
> 
> It's a simple condition, so we can just remove this when the other patch
> lands? Currently this will cause a crash, and I don't think we want to keep
> it in its current state on trunk. I can add a FIXME if that helps.

I've tried removing this piece of code from the patch (attachment 434041 [details]), applied on top of r280256, and no crash is observed when running JSTests/stress/reflect-set-custom-value.js via `run-javascriptcore-tests`. I believe the crash is due to non-cleared CustomValue attribute, and not due to custom setter not called.

> >> Source/JavaScriptCore/runtime/JSObject.cpp:-931
> >> -            putDirect(vm, propertyName, value, attributesForStructure(entry->value->attributes()) & ~PropertyAttribute::CustomValue, slot);
> > 
> > This won't be needed if you apply the comment before previous.
> 
> I think it's still relevant. We never want to end up here with a custom value.

Yeah, sorry, I've worded poorly.
We can't remove `putDirect(... ~PropertyAttribute::CustomValue)` bit, yet we won't need to change it to an ASSERT.

> >> Source/JavaScriptCore/runtime/JSObjectInlines.h:-285
> >> -    // FIXME: For a failure due to non-extensible structure, the error message is misleading.
> > 
> > There are quite a few FIXMEs like this; maybe we should tackle all of them in separate change to keep this (very tricky) patch simpler.
> 
> I see... these should have had a bug to make it easier to track them.

Agreed, it's totally my fault: I didn't file the bug, hoping to do a quick follow-up.

> If I don't fix this particular instance, it breaks a couple tests, but I guess I can skip the tests in the mean time.

JSObject::putInlineFast() is kinda hot; is it possible that adding a code to a branch that rarely executes can hit inlining threshold somewhere?
In a future, maybe we should return the failure reason from putDirectInternal() or even expand ReadonlyPropertyWriteError to say about non-extensible structure?

[Tadeu Zagallo]

Created attachment 434579 [details]
Patch

[Alexey Shvayka]

Comment on attachment 434579 [details]
Patch

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

r=me with comments. Nicely done!

> Source/JavaScriptCore/runtime/JSObject.cpp:884
> +                return customSetter(globalObject, JSValue::encode(receiver), JSValue::encode(value), propertyName);

1. After r280256, custom properties are called in realm of their holder, like `customSetter(receiver->globalObject(vm), ...`.
2. `customSetter` can declare a throw scope, causing unchecked scope error in --debug. I suggest `RELEASE_AND_RETURN(scope, customSetter...`.

> Source/JavaScriptCore/runtime/JSObject.cpp:942
> +            ASSERT(!(entry->value->attributes() & PropertyAttribute::CustomValue));

I've took a brief look and it seems like this ASSERT will hold for putInlineFastReplacingStaticPropertyIfNeeded() called by JSObject::putInlineForJSObject(). Nice!

> Source/JavaScriptCore/runtime/JSObjectInlines.h:-285
> -    // FIXME: For a failure due to non-extensible structure, the error message is misleading.

Maybe we should keep this FIXME as it's not yet resolved?

[Tadeu Zagallo]

Created attachment 434594 [details]
Patch for landing

[EWS]

Tools/Scripts/svn-apply failed to apply attachment 434594 [details] to trunk.
Please resolve the conflicts and upload a new patch.

[Tadeu Zagallo]

Created attachment 434596 [details]
Patch for landing

[EWS]

Committed r280463 (240098@main): <https://commits.webkit.org/240098@main>

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

[Alexey Shvayka]

Comment on attachment 434596 [details]
Patch for landing

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

> Source/JavaScriptCore/runtime/JSObject.cpp:940
> +            ASSERT(!(entry->value->attributes() & PropertyAttribute::CustomValue));

This ASSERT appears to be failing: please see https://build.webkit.org/#/builders/100/builds/630.
I suggest we revert the change to putInlineFastReplacingStaticPropertyIfNeeded() because definePropertyOnReceiver() handles CustomValues properties only on structure.

[Alexey Shvayka]

(In reply to Alexey Shvayka from comment #21)
> This ASSERT appears to be failing: please see
> https://build.webkit.org/#/builders/100/builds/630.
> I suggest we revert the change to
> putInlineFastReplacingStaticPropertyIfNeeded() because
> definePropertyOnReceiver() handles CustomValues properties only on structure.

Also, it would be nice if we could come up with a test that is currently crashing; this might be an issue similar to the fixed one, but for non-reified CustomValue.

[Tadeu Zagallo]

Reopening to attach new patch.

[Tadeu Zagallo]

Created attachment 434676 [details]
Patch

[Alexey Shvayka]

Comment on attachment 434676 [details]
Patch

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

Thank you for the fix! I've failed to see this case during review.
r=me with a comment.

> Source/JavaScriptCore/runtime/JSObject.cpp:939
> +            if (entry->value->attributes() & PropertyAttribute::CustomValue) {

I hope we would remove this in near feature as there is no use cases for calling the setter except JSDollarVM.cpp.

> Source/JavaScriptCore/runtime/JSObject.cpp:942
> +                    RELEASE_AND_RETURN(scope, customSetter(globalObject, JSValue::encode(this), JSValue::encode(value), propertyName));

For consistency: `... customSetter(structure->globalObject(), ...`

[Tadeu Zagallo]

Comment on attachment 434676 [details]
Patch

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

Thanks for the review!

>> Source/JavaScriptCore/runtime/JSObject.cpp:939
>> +            if (entry->value->attributes() & PropertyAttribute::CustomValue) {
> 
> I hope we would remove this in near feature as there is no use cases for calling the setter except JSDollarVM.cpp.

absolutely, that'd be great.

>> Source/JavaScriptCore/runtime/JSObject.cpp:942
>> +                    RELEASE_AND_RETURN(scope, customSetter(globalObject, JSValue::encode(this), JSValue::encode(value), propertyName));
> 
> For consistency: `... customSetter(structure->globalObject(), ...`

I thought about it, but somehow got confused and thought it wasn't necessary... thanks for catching it.

[Tadeu Zagallo]

Created attachment 434678 [details]
Patch for landing (2)

[Tadeu Zagallo]

Created attachment 434679 [details]
Patch for landing (2)

[Alexey Shvayka]

(In reply to Tadeu Zagallo from comment #26)
> >> Source/JavaScriptCore/runtime/JSObject.cpp:942
> >> +                    RELEASE_AND_RETURN(scope, customSetter(globalObject, JSValue::encode(this), JSValue::encode(value), propertyName));
> > 
> > For consistency: `... customSetter(structure->globalObject(), ...`
> 
> I thought about it, but somehow got confused and thought it wasn't
> necessary... thanks for catching it.

Yeah, it might seem that way, yet `globalObject` coming from put() override is lexical so the [[Set]] errors are thrown in expected realm.

In WebCore, `lexicalGlobalObject` naming is used to avoid confusion.

[EWS]

Patch 434678 does not build

[EWS]

Committed r280505 (240137@main): <https://commits.webkit.org/240137@main>

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