Bug 206846 - [JSC] Give up IC when unknown structure transition happens
Summary: [JSC] Give up IC when unknown structure transition happens
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-01-27 14:53 PST by Yusuke Suzuki
Modified: 2020-01-29 15:02 PST (History)
9 users (show)

See Also:


Attachments
Patch (12.12 KB, patch)
2020-01-27 14:55 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (12.04 KB, patch)
2020-01-27 15:43 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (44.64 KB, patch)
2020-01-28 17:46 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (61.62 KB, patch)
2020-01-28 18:23 PST, Yusuke Suzuki
mark.lam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2020-01-27 14:53:19 PST
...
Comment 1 Yusuke Suzuki 2020-01-27 14:54:54 PST
<rdar://problem/58091768>
Comment 2 Yusuke Suzuki 2020-01-27 14:55:54 PST
Created attachment 388919 [details]
Patch

EWS stressing
Comment 3 Yusuke Suzuki 2020-01-27 15:43:19 PST
Created attachment 388929 [details]
Patch

EWS stressing
Comment 4 Yusuke Suzuki 2020-01-28 17:46:37 PST
Created attachment 389096 [details]
Patch
Comment 5 Yusuke Suzuki 2020-01-28 18:23:57 PST
Created attachment 389099 [details]
Patch
Comment 6 Mark Lam 2020-01-28 20:26:44 PST
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?
Comment 7 Yusuke Suzuki 2020-01-29 09:59:06 PST
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.
Comment 8 Yusuke Suzuki 2020-01-29 10:07:45 PST
Committed r255365: <https://trac.webkit.org/changeset/255365>
Comment 10 Yusuke Suzuki 2020-01-29 15:02:41 PST
Committed r255390: <https://trac.webkit.org/changeset/255390>