Bug 154009 - GetValueFunc/PutValueFunc should not take both slotBase and thisValue
Summary: GetValueFunc/PutValueFunc should not take both slotBase and thisValue
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Gavin Barraclough
URL:
Keywords:
Depends on: 154064
Blocks:
  Show dependency treegraph
 
Reported: 2016-02-08 14:18 PST by Gavin Barraclough
Modified: 2016-02-10 07:41 PST (History)
3 users (show)

See Also:


Attachments
Fix (280.47 KB, patch)
2016-02-09 00:28 PST, Gavin Barraclough
no flags Details | Formatted Diff | Diff
With style fix (280.47 KB, patch)
2016-02-09 00:37 PST, Gavin Barraclough
no flags Details | Formatted Diff | Diff
Again (281.75 KB, patch)
2016-02-09 11:44 PST, Gavin Barraclough
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gavin Barraclough 2016-02-08 14:18:11 PST
In JavaScript there are two types of properties - regular value properties, and accessor properties. One difference between these is how they are reflected by getOwnPropertyDescriptor, and another is what object they operate on in the case of a prototype access. If you access a value property of a prototype object it return a value pertinent to the prototype, but in the case of a prototype object returning an accessor, then the accessor function is applied to the base object of the access.

JSC supports special 'custom' properties implemented as a c++ callback, and these custom properties can be used to implement either value- or accessor-like behavior. getOwnPropertyDescriptor behavior is selected via the CustomAccessor attribute. Value- or accessor-like object selection is current supported by passing both the slotBase and the thisValue to the callback,and hoping it uses the right one. This is probably inefficient, bug-prone, and leads to crazy like JSBoundSlotBaseFunction.

Instead, just pass one thisValue to the callback functions, consistent with CustomAccessor.
Comment 1 Gavin Barraclough 2016-02-09 00:28:20 PST
Created attachment 270914 [details]
Fix
Comment 2 WebKit Commit Bot 2016-02-09 00:29:39 PST
Attachment 270914 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/CustomGetterSetter.h:73:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 59 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Gavin Barraclough 2016-02-09 00:32:28 PST
As a follow up to this, more cleanup around JSBoundSlotBaseFunction/CustomGetter should be possible.

JSBoundSlotBaseFunction no longer needs to bind the slotBase (it's never used!), And we can probably merge these back with CustomGetters (reify of value/accessor properties always just creates a CustomGetter; lazily create a JSFunction derivate only if requested by getOwnPropertyDescriptor).
Comment 4 Gavin Barraclough 2016-02-09 00:37:31 PST
Created attachment 270915 [details]
With style fix
Comment 5 Gavin Barraclough 2016-02-09 11:44:52 PST
Created attachment 270939 [details]
Again
Comment 6 Geoffrey Garen 2016-02-09 12:55:11 PST
Comment on attachment 270939 [details]
Again

r=me
Comment 7 Gavin Barraclough 2016-02-09 13:22:32 PST
Fixed in r196331
Comment 8 Csaba Osztrogonác 2016-02-10 04:41:40 PST
(In reply to comment #7)
> Fixed in r196331

It made ~180 JSC tests crash on ARMv7 Linux bots: bug154064

Do you think if it is a Linux specific bug or have you noticed
this regression on your internal iOS ARM JSC tester bots too?
Comment 9 Csaba Osztrogonác 2016-02-10 07:41:21 PST
(In reply to comment #8)
> (In reply to comment #7)
> > Fixed in r196331
> 
> It made ~180 JSC tests crash on ARMv7 Linux bots: bug154064
> 
> Do you think if it is a Linux specific bug or have you noticed
> this regression on your internal iOS ARM JSC tester bots too?

I found the bug and fixed it in bug154064. The problem is related to ARM EABI,
which says that 64 bits value should be aligned to even numbered register.