Bug 173057

Summary: [JSC] has_generic_property never accepts non-String
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: New BugsAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, darin, keith_miller, mark.lam, msaboff, saam
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch darin: review+

Yusuke Suzuki
Reported 2017-06-07 07:14:28 PDT
[JSC] has_generic_property never accepts non-String
Attachments
Patch (1.84 KB, patch)
2017-06-07 07:14 PDT, Yusuke Suzuki 		no flags
DetailsFormatted DiffDiff
Patch (1.89 KB, patch)
2017-06-07 07:18 PDT, Yusuke Suzuki 		darin: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Yusuke Suzuki
Comment 1 2017-06-07 07:14:56 PDT
Created attachment 312181 [details] Patch
Yusuke Suzuki
Comment 2 2017-06-07 07:18:06 PDT
Created attachment 312182 [details] Patch
Darin Adler
Comment 3 2017-06-07 09:00:36 PDT
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 4 2017-06-07 09:01:07 PDT
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 5 2017-06-07 09:01:54 PDT
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 6 2017-06-07 09:53:16 PDT
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
Comment 7 2017-06-07 09:54:35 PDT
Committed r217887: <http://trac.webkit.org/changeset/217887>
Note You need to log in before you can comment on or make changes to this bug.