Bug 213266 – [JSC] Check NullSetterFunction under strict-mode context since structure / PropertyCondition are unaware of this

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

[Yusuke Suzuki]

...

[Yusuke Suzuki]

<rdar://problem/63827506>

[Yusuke Suzuki]

Created attachment 402056 [details]
Patch

[Mark Lam]

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?

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Committed r263134: <https://trac.webkit.org/changeset/263134>

[Yusuke Suzuki]

Committed r263165: <https://trac.webkit.org/changeset/263165>