Bug 213266

Summary: [JSC] Check NullSetterFunction under strict-mode context since structure / PropertyCondition are unaware of this
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: JavaScriptCoreAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: ews-watchlist, keith_miller, mark.lam, msaboff, saam, tzagallo, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch mark.lam: review+

Description Yusuke Suzuki 2020-06-16 16:02:31 PDT
...
Comment 1 Yusuke Suzuki 2020-06-16 16:08:10 PDT
<rdar://problem/63827506>
Comment 2 Yusuke Suzuki 2020-06-16 17:04:09 PDT
Created attachment 402056 [details]
Patch
Comment 3 Mark Lam 2020-06-16 19:11:43 PDT
Comment on attachment 402056 [details]
Patch

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

r=me

> Source/JavaScriptCore/ChangeLog:26
> +        we replace the calling target with special function which throws an error. In DFG / FTL, we emit `CheckNotJSCast` DFT node which

typo: /DFT node/DFG node/

> Source/JavaScriptCore/bytecode/AccessCase.cpp:1606
> +            // Note that GetterSetter alwyas has cells for both. If it is not set (like, getter exits, but setter is not set), Null{Getter,Setter}Function is stored.

typo: /alwyas/always/

> Source/JavaScriptCore/bytecode/AccessCase.cpp:1614
> +                    // We replace getter with this AccessCase's JSGlobalObject::nullSetterStrictFunction, which will throw an error with the right JSGlobalObject.

You're replacing the "setter" here, not the ""getter", right?

> Source/JavaScriptCore/dfg/DFGStructureAbstractValue.cpp:358
> +    // Note taht this function returns true if the structure set is empty.

typo: /taht/that/.  Same for StructureAbstractValue::isSubClassOf above.

> Source/JavaScriptCore/runtime/NullSetterFunction.cpp:93
> +    constexpr bool useCurrentFrame = false;

At first I thought this is wrong, but then, I realized that this function is an InternalFunction, and therefore should be skipped.  Can you write a short test to verify that error.stack's top frame entry is that of the JS function exercising the IC?  Or augment one of your existing tests?
Comment 4 Yusuke Suzuki 2020-06-16 19:14:13 PDT
Comment on attachment 402056 [details]
Patch

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

>> Source/JavaScriptCore/bytecode/AccessCase.cpp:1606
>> +            // Note that GetterSetter alwyas has cells for both. If it is not set (like, getter exits, but setter is not set), Null{Getter,Setter}Function is stored.
> 
> typo: /alwyas/always/

Fixed.

>> Source/JavaScriptCore/bytecode/AccessCase.cpp:1614
>> +                    // We replace getter with this AccessCase's JSGlobalObject::nullSetterStrictFunction, which will throw an error with the right JSGlobalObject.
> 
> You're replacing the "setter" here, not the ""getter", right?

Fixed.

>> Source/JavaScriptCore/dfg/DFGStructureAbstractValue.cpp:358
>> +    // Note taht this function returns true if the structure set is empty.
> 
> typo: /taht/that/.  Same for StructureAbstractValue::isSubClassOf above.

Fixed.

>> Source/JavaScriptCore/runtime/NullSetterFunction.cpp:93
>> +    constexpr bool useCurrentFrame = false;
> 
> At first I thought this is wrong, but then, I realized that this function is an InternalFunction, and therefore should be skipped.  Can you write a short test to verify that error.stack's top frame entry is that of the JS function exercising the IC?  Or augment one of your existing tests?

Added.
Comment 5 Yusuke Suzuki 2020-06-16 19:30:31 PDT
Committed r263134: <https://trac.webkit.org/changeset/263134>
Comment 6 Yusuke Suzuki 2020-06-17 10:31:11 PDT
Committed r263165: <https://trac.webkit.org/changeset/263165>