Bug 206846 – [JSC] Give up IC when unknown structure transition happens

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

[Yusuke Suzuki]

...

[Yusuke Suzuki]

<rdar://problem/58091768>

[Yusuke Suzuki]

Created attachment 388919 [details]
Patch

EWS stressing

[Yusuke Suzuki]

Created attachment 388929 [details]
Patch

EWS stressing

[Yusuke Suzuki]

Created attachment 389096 [details]
Patch

[Yusuke Suzuki]

Created attachment 389099 [details]
Patch

[Mark Lam]

Comment on attachment 389099 [details]
Patch

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

r=me

> Source/JavaScriptCore/ChangeLog:12
> +        When we are creating Put IC for new property, we grab old Structure before performing put.
> +        Our custom ::put has convention that implemented ::put mark PutPropertySlot non-cachable
> +        appropriately so that IC can work correctly. If we miss this, semantic failure can happen.
> +        This patch hardens this semantic failure case by giving up IC when newStructure calculated
> +        from oldStructure does not match against the actual structure.

I suggest a few edits for clarity:

When we are creating Put IC for a new property, we grab the old Structure before performing the put.
For a custom ::put, our convention is that the implemented ::put should mark the PutPropertySlot 
as non-cachable.  The IC code relies on this in order to work correctly.  If we didn't mark it as non-cacheable,
a semantic failure can happen.  This patch hardens the code against this semantic failure case by giving up
trying to cache the IC when the newStructure calculated from oldStructure does not match against the actual 
structure after the put operation.

> Source/WebCore/ChangeLog:10
> +        when it implements custom ::put operation which has side-effect regardless of Structure. IC could be setup
> +        and IC can do fast path without consulting to custom ::put operation.

"Structure. IC could be setup" => "Structure. Otherwise, IC can be setup"
"without consulting to custom" => ""without consulting the custom"

> Source/JavaScriptCore/jit/Repatch.cpp:615
> +                // If JSObject::put is overridden by UserObject, UserObject::put performs side-effect on JSObject::put, and it forget making PutPropertySlot non-cachaeble,
> +                // then arbitrary structure transitions can happen during the put operation, and this generates wrong transition information here, oldStructure -> newStructure,
> +                // while the reality is oldStructure -> something unknown structures -> baseValue's structure. We should check, and give up to guard against the incorrect embedder's
> +                // UserObject::put implementation.

Some suggestions:

"it forget making PutPropertySlot" => "it neglects to mark the PutPropertySlot as"
"wrong transition information here, oldStructure -> newStructure" => "wrong transition information here as if oldStructure -> newStructure."
"while the reality is" => "In reality, the transition is"
"We should check, and give up to guard against the incorrect embedder's UserObject::put implementation." => "To guard against the embedder's potentially incorrect UserObject::put implementation, we should check for this condition and if found, give up on caching the put."

> Source/JavaScriptCore/tools/JSDollarVM.cpp:2304
> +    auto* dollarVM = jsDynamicCast<JSDollarVM*>(vm, callFrame->thisValue());
> +    if (!dollarVM)
> +        return JSValue::encode(jsNull());

Do you expect the function to be called with a this value that is not $vm?  I'm guessing you're trying to prevent unauthorized use of this function.  I think you don't need this.  The DollarVMAssertScope above already ensures that JSC_useDollarVM is true, i.e. the client has authorization to call this function.  Since tests that call this function are always well-formed, we don't need this check.  Though if you want to keep it, that's fine too.

> JSTests/stress/ensure-crash.js:1
> +//@ crash!

I suggest adding a //@ runDefault line before this.  There's no point in running this test on all flavors.  We just need to check it once, right?

[Yusuke Suzuki]

Comment on attachment 389099 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:12
>> +        from oldStructure does not match against the actual structure.
> 
> I suggest a few edits for clarity:
> 
> When we are creating Put IC for a new property, we grab the old Structure before performing the put.
> For a custom ::put, our convention is that the implemented ::put should mark the PutPropertySlot 
> as non-cachable.  The IC code relies on this in order to work correctly.  If we didn't mark it as non-cacheable,
> a semantic failure can happen.  This patch hardens the code against this semantic failure case by giving up
> trying to cache the IC when the newStructure calculated from oldStructure does not match against the actual 
> structure after the put operation.

Fixed.

>> Source/WebCore/ChangeLog:10
>> +        and IC can do fast path without consulting to custom ::put operation.
> 
> "Structure. IC could be setup" => "Structure. Otherwise, IC can be setup"
> "without consulting to custom" => ""without consulting the custom"

Fixed.

>> Source/JavaScriptCore/jit/Repatch.cpp:615
>> +                // UserObject::put implementation.
> 
> Some suggestions:
> 
> "it forget making PutPropertySlot" => "it neglects to mark the PutPropertySlot as"
> "wrong transition information here, oldStructure -> newStructure" => "wrong transition information here as if oldStructure -> newStructure."
> "while the reality is" => "In reality, the transition is"
> "We should check, and give up to guard against the incorrect embedder's UserObject::put implementation." => "To guard against the embedder's potentially incorrect UserObject::put implementation, we should check for this condition and if found, give up on caching the put."

Looks nice, fixed.

>> Source/JavaScriptCore/tools/JSDollarVM.cpp:2304
>> +        return JSValue::encode(jsNull());
> 
> Do you expect the function to be called with a this value that is not $vm?  I'm guessing you're trying to prevent unauthorized use of this function.  I think you don't need this.  The DollarVMAssertScope above already ensures that JSC_useDollarVM is true, i.e. the client has authorization to call this function.  Since tests that call this function are always well-formed, we don't need this check.  Though if you want to keep it, that's fine too.

Or, we can just put RELEASE_ASSERT here. Fixed.

>> JSTests/stress/ensure-crash.js:1
>> +//@ crash!
> 
> I suggest adding a //@ runDefault line before this.  There's no point in running this test on all flavors.  We just need to check it once, right?

No, we have the same bug in LLInt and Baseline. So we should execute useLLInt=false and LLInt case at least.

[Yusuke Suzuki]

Committed r255365: <https://trac.webkit.org/changeset/255365>

[Truitt Savell]

It looks like this change could have caused 32 JSC failures:
https://build.webkit.org/builders/Apple-Catalina-Debug-JSC-Tests/builds/223

stdio:
https://build.webkit.org/builders/Apple-Catalina-Debug-JSC-Tests/builds/223/steps/jscore-test/logs/stdio

[Yusuke Suzuki]

Committed r255390: <https://trac.webkit.org/changeset/255390>