Bug 144265 – JSCallbackObject does not maintain symmetry between accesses for getOwnPropertySlot and put

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

[Saam Barati]

The ordering of the 'if' statements inside JSScope.cpp's abstractAccess need some rejiggering.

[Saam Barati]

Created attachment 251758 [details]
the patch

[Geoffrey Garen]

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.

[Saam Barati]

Created attachment 251774 [details]
patch

[Saam Barati]

(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.

[Saam Barati]

Created attachment 251797 [details]
the patch

[Geoffrey Garen]

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?

[Saam Barati]

(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?

[Geoffrey Garen]

> 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.

[Geoffrey Garen]

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.

[Geoffrey Garen]

(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.

[Geoffrey Garen]

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.

[WebKit Commit Bot]

Comment on attachment 251797 [details]
the patch

Clearing flags on attachment: 251797

Committed r183754: <http://trac.webkit.org/changeset/183754>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Saam Barati]

(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