Bug 173057 – [JSC] has_generic_property never accepts non-String

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

[JSC] has_generic_property never accepts non-String

[Yusuke Suzuki]

Created attachment 312181 [details]
Patch

[Yusuke Suzuki]

Created attachment 312182 [details]
Patch

[Darin Adler]

Comment on attachment 312182 [details]
Patch

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

> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:717
> +    ASSERT(property.isString());
> +    RETURN(jsBoolean(base->hasPropertyGeneric(exec, asString(property.asCell())->toIdentifier(exec), PropertySlot::InternalMethodType::GetOwnProperty)));

The asString function already does this assertion; separately asserting is unnecessary.

[Darin Adler]

Comment on attachment 312182 [details]
Patch

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

>> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:717
>> +    RETURN(jsBoolean(base->hasPropertyGeneric(exec, asString(property.asCell())->toIdentifier(exec), PropertySlot::InternalMethodType::GetOwnProperty)));
> 
> The asString function already does this assertion; separately asserting is unnecessary.

Style-wise,  would suggest making the local variable a JSString* and doing the asString on the separate line.

[Darin Adler]

Comment on attachment 312182 [details]
Patch

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

>>> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:717
>>> +    RETURN(jsBoolean(base->hasPropertyGeneric(exec, asString(property.asCell())->toIdentifier(exec), PropertySlot::InternalMethodType::GetOwnProperty)));
>> 
>> The asString function already does this assertion; separately asserting is unnecessary.
> 
> Style-wise,  would suggest making the local variable a JSString* and doing the asString on the separate line.

But also wondering if we need to check for exception after calling toIdentifier and before calling hasPropertyGeneric, in case toIdentifier runs into a memory issue resolving a rope.

[Yusuke Suzuki]

Comment on attachment 312182 [details]
Patch

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

>>>> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:717
>>>> +    RETURN(jsBoolean(base->hasPropertyGeneric(exec, asString(property.asCell())->toIdentifier(exec), PropertySlot::InternalMethodType::GetOwnProperty)));
>>> 
>>> The asString function already does this assertion; separately asserting is unnecessary.
>> 
>> Style-wise,  would suggest making the local variable a JSString* and doing the asString on the separate line.
> 
> But also wondering if we need to check for exception after calling toIdentifier and before calling hasPropertyGeneric, in case toIdentifier runs into a memory issue resolving a rope.

Sounds nice. We should insert check here. Fixed.

[Yusuke Suzuki]

Committed r217887: <http://trac.webkit.org/changeset/217887>