Bug 213266 - [JSC] Check NullSetterFunction under strict-mode context since structure / PropertyCondition are unaware of this
Summary: [JSC] Check NullSetterFunction under strict-mode context since structure / Pr...
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-06-16 16:02 PDT by Yusuke Suzuki
Modified: 2020-06-17 10:31 PDT (History)
7 users (show)

See Also:


Attachments
Patch (52.29 KB, patch)
2020-06-16 17:04 PDT, 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-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>