Bug 144265 - JSCallbackObject does not maintain symmetry between accesses for getOwnPropertySlot and put
Summary: JSCallbackObject does not maintain symmetry between accesses for getOwnProper...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Saam Barati
URL:
Keywords:
Depends on:
Blocks: 144178
  Show dependency treegraph
 
Reported: 2015-04-27 10:32 PDT by Saam Barati
Modified: 2015-05-04 19:49 PDT (History)
7 users (show)

See Also:


Attachments
the patch (2.80 KB, patch)
2015-04-27 10:59 PDT, Saam Barati
ggaren: review-
Details | Formatted Diff | Diff
patch (4.31 KB, patch)
2015-04-27 13:33 PDT, Saam Barati
sbarati: review-
sbarati: commit-queue-
Details | Formatted Diff | Diff
the patch (3.72 KB, patch)
2015-04-27 17:48 PDT, Saam Barati
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Saam Barati 2015-04-27 10:32:46 PDT
The ordering of the 'if' statements inside JSScope.cpp's abstractAccess need some rejiggering.
Comment 1 Saam Barati 2015-04-27 10:59:13 PDT
Created attachment 251758 [details]
the patch
Comment 2 Geoffrey Garen 2015-04-27 11:22:53 PDT
Comment on attachment 251758 [details]
the patch

Needs a test case.

I think when you try to write a test case you will discover that the current behavior is correct because symbol table properties have higher priority than structure properties, and so they can be cached even in the presence of weird structure properties.
Comment 3 Saam Barati 2015-04-27 13:33:05 PDT
Created attachment 251774 [details]
patch
Comment 4 Saam Barati 2015-04-27 17:41:37 PDT
(In reply to comment #2)
> Comment on attachment 251758 [details]
> the patch
> 
> Needs a test case.
> 
> I think when you try to write a test case you will discover that the current
> behavior is correct because symbol table properties have higher priority
> than structure properties, and so they can be cached even in the presence of
> weird structure properties.

Yeah. I think you're right. The bug lies in JSCallbackObject.

The code for JSCallbackObject::staticFunctionGetter will check if the parent class has the given property slot. If the parent does, it will return that, otherwise, it will return what it has. ::put does not do this. ::put will always put static functions onto the JSCallbackObject itself, even when the Parent class has the property. I think there is a lack of symmetry here that should be solved by having ::put check to see if the parent has the given property slot, and if so, deferring to the parent's ::put implementation.
Comment 5 Saam Barati 2015-04-27 17:48:44 PDT
Created attachment 251797 [details]
the patch
Comment 6 Geoffrey Garen 2015-04-28 10:59:00 PDT
Comment on attachment 251797 [details]
the patch

Hmmm... why do this for only static functions and not static values?

Should we always do this, or only do this when we subclass the global object, which has a symbol table?
Comment 7 Saam Barati 2015-04-28 11:21:45 PDT
(In reply to comment #6)
> Comment on attachment 251797 [details]
> the patch
> 
> Hmmm... why do this for only static functions and not static values?
> 
> Should we always do this, or only do this when we subclass the global
> object, which has a symbol table?

Good questions. I'm not sure on all counts because I'm not well versed on what specifically JSCallbackObject 
is used for and the subtleties that arise. 

I think depending on what we decide, we should regardless make put/get be symmetrical. 
It shouldn't be that case that a put followed by an immediate get doesn't return the value just put:
- put(X : V); get(X) == V

Looking at the code for static values, it looks like it's always deferred to the 
JSClassDefinition's implementation of 'setProperty' and 'getProperty', and if these
don't exist or fail, it falls back to the parent implementation.

Just by reading the code, it seems symmetrical. It's weird thought that it doesn't
give the parent class priority where static functions do give the parent class priority.

Regarding giving the parent's implementation priority only with the global object, 
what other contexts is JSCallbackObject used for?
Comment 8 Geoffrey Garen 2015-05-04 10:52:44 PDT
> Regarding giving the parent's implementation priority only with the global
> object, 
> what other contexts is JSCallbackObject used for?

JSCallbackObject is used for clients of the API. "Callback" was my not-very-good-name for "I'm implemented as a set of C callbacks".

JSObjectMake will make a JSCallbackObject<JSNonFinalObject>.

JSGlobalContextCreate with a custom JSClassRef for the global object will make a JSCallbackObject<JSGlobalObject>. This allows the API client to override global properties like JSDOMWindow does.

It's only in the JSCallbackObject<JSGlobalObject> case that you have a symbol table in your base class.
Comment 9 Geoffrey Garen 2015-05-04 10:56:26 PDT
It looks like JSCallbackObject<Parent>::staticFunctionGetter currently defers to the base class in get for the sake of caching. (A comment says "Check for cached or override property.") So, the first access will allocate a function and put() it, and the second access will get() the function to avoid allocating it again.
Comment 10 Geoffrey Garen 2015-05-04 10:57:31 PDT
(In reply to comment #9)
...This has the side effect of allowing the base C++ class to override the property entirely, which will happen in the global object case if you declare a var with that property name.
Comment 11 Geoffrey Garen 2015-05-04 10:58:21 PDT
Comment on attachment 251797 [details]
the patch

r=me

OK, I suppose it's nice to be symmetrical.

Perhaps we should extend this behavior to static values as well in a separate patch.
Comment 12 WebKit Commit Bot 2015-05-04 11:48:52 PDT
Comment on attachment 251797 [details]
the patch

Clearing flags on attachment: 251797

Committed r183754: <http://trac.webkit.org/changeset/183754>
Comment 13 WebKit Commit Bot 2015-05-04 11:48:56 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 Saam Barati 2015-05-04 19:49:00 PDT
(In reply to comment #11)
> Comment on attachment 251797 [details]
> the patch
> 
> r=me
> 
> OK, I suppose it's nice to be symmetrical.
> 
> Perhaps we should extend this behavior to static values as well in a
> separate patch.

Opened a bug to consider this:

https://bugs.webkit.org/show_bug.cgi?id=144618