Bug 144265

Created attachment 251758 [details] the patch

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.

Created attachment 251774 [details] patch

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

Created attachment 251797 [details] the patch

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?

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

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

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.

(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 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 on attachment 251797 [details] the patch Clearing flags on attachment: 251797 Committed r183754: <http://trac.webkit.org/changeset/183754>

All reviewed patches have been landed. Closing bug.

(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