Bug 33603 – Some window properties are not returned by Object.keys and Object.getOwnPropertyNames

• Component: WebCore JavaScript
• Status: RESOLVED INVALID

[Kent Hansen]

For example, the window object has a property called "addEventListener";
window.hasOwnProperty("addEventListener") returns true, but
Object.keys(window).indexOf("addEventListener") returns -1.
The property _is_ returned by a for-in enumeration.

[Kent Hansen]

Created attachment 46914 [details]
Proposed patch

Make getOwnPropertyNames() more consistent with getOwnPropertyDescriptor().

I'm not sure if it would rather make sense to change the JSDOMWindow::getOwnPropertyDescriptor() implementation to not look in the prototype, and then change JSObject::hasOwnProperty() to call getOwnPropertyDescriptor(). It's fine that getOwnPropertySlot() does the proxying when actually accessing the property (since that shouldn't be observable from the outside), but should getOwnPropertyDescriptor() really do it as well? It does feel a bit odd that e.g. window.hasOwnProperty("__defineGetter__") returns true.

[Darin Adler]

Comment on attachment 46914 [details]
Proposed patch

Your questions about what behavior we want are good ones. I’m not sure we should land this since it seems it may be going in the wrong direction.

[Kent Hansen]

(In reply to comment #2)
> (From update of attachment 46914 [details])
> Your questions about what behavior we want are good ones. I’m not sure we
> should land this since it seems it may be going in the wrong direction.

Yep. I have other patches that are impacted by this behavior, so it would be nice to know what behavior should be the sanctioned one.

[Darin Adler]

The people I’d ask about this are Geoff Garen, Sam Weinig, and Maciej Stachowiak.

[Darin Adler]

And Oliver Hunt.

[Geoffrey Garen]

Comment on attachment 46914 [details]
Proposed patch

I don't think we want to make getOwnPropertyNames include prototype properties -- that's moving in the wrong direction, since "own" specifically means "not prototype".

[Geoffrey Garen]

> I'm not sure if it would rather make sense to change the
> JSDOMWindow::getOwnPropertyDescriptor() implementation to not look in the
> prototype, and then change JSObject::hasOwnProperty() to call
> getOwnPropertyDescriptor().

Yes, I think that's the right solution, since it honors the meaning of "own" as "not prototype".

[Kent Hansen]

(In reply to comment #7)
> > I'm not sure if it would rather make sense to change the
> > JSDOMWindow::getOwnPropertyDescriptor() implementation to not look in the
> > prototype, and then change JSObject::hasOwnProperty() to call
> > getOwnPropertyDescriptor().
> 
> Yes, I think that's the right solution, since it honors the meaning of "own" as
> "not prototype".

Thanks, closing this task and pursuing the suggested solution.

[Kent Hansen]

(In reply to comment #8)
> (In reply to comment #7)
> > > I'm not sure if it would rather make sense to change the
> > > JSDOMWindow::getOwnPropertyDescriptor() implementation to not look in the
> > > prototype, and then change JSObject::hasOwnProperty() to call
> > > getOwnPropertyDescriptor().
> > 
> > Yes, I think that's the right solution, since it honors the meaning of "own" as
> > "not prototype".
> 
> Thanks, closing this task and pursuing the suggested solution.

The problem with changing JSObject::hasOwnProperty() to call getOwnPropertyDescriptor() is that JSCallbackObject doesn't reimplement getOwnPropertyDescriptor() (only getOwnPropertySlot()), so hasOwnProperty() would stop working if you're using the C API and have a hasProperty callback installed. No go.

I think JSCallbackObject should reimplement getOwnPropertyDescriptor(), but what should it do?
 - It can call the hasProperty callback to figure out whether there is such a property; OK.
 - It's an accessor property, so for the [[Get]] and [[Set]] fields it should create function objects that wrap the getProperty and setProperty callbacks? Should those be cached, then? Or should getOwnPropertyDescriptor() just call the getter and pretend that it's a value property?
 - If it's a static value property, the attributes are available. Otherwise, the property is assumed to be enumerable and configurable? (Calling the getPropertyNames callback to deduce whether the property is enumerable seems overkill.)

I'd like to implement this if you think it makes sense (since it's a stopper for the JSObject::hasOwnProperty change, which is a stopper for the JSDOMWindow::getOwnPropertyDescriptor change :) ).

[Kent Hansen]

(In reply to comment #9)
> I think JSCallbackObject should reimplement getOwnPropertyDescriptor(), but
> what should it do?

I went ahead and did a prototype implementation.
- Add two helper classes, JSGetPropertyCallbackFunction and JSSetPropertyCallbackFunction, that store a property name and callback function, and whose call() implementation delegates to the callback (very similar to JSCallbackFunction).
- Implement JSCallbackObject::getOwnPropertyDescriptor() to look for the property in the JSClass and create callback wrappers that are returned in the descriptor.
- I didn't implement the caching yet, but I think it can be done similarly to JSCallbackObject::staticFunctionGetter() (use lookupGetter()/defineGetter()?).

With that, I'm able to introspect JSClass-based objects from JS using Object.getOwnPropertyDescriptor.

I can spin off a task and post the patch there if the above approach seems reasonable.
Having to wrap the callbacks sure complicates things vs calling the getter in-place and returning a value descriptor, but the latter would be lying.

[Geoffrey Garen]

> I think JSCallbackObject should reimplement getOwnPropertyDescriptor()

Me too.

But let's separate out two concerns.

Concern 1: Switching hasOwnProperty to use getOwnPropertyDescriptor would break hasOwnProperty for API objects.

Concern 2: getOwnPropertyDescriptor has never worked very well for API objects.

Concern 1 is easy to solve: Have JSCallbackObject::getOwnPropertyDescriptor call JSCallbackObject:: getOwnPropertySlot and return a value descriptor, and add a hasOwnProperty and getOwnPropertyDescriptor test to testapi.c. That maintains current behavior for hasOwnProperty. It also solves a bit of Concern 2, because getOwnPropertyDescriptor will stop returning 'undefined' for API properties.

I think your more ambitious proposal for solving Concern 2 sounds reasonable, but Concern 2 is a sidetrack from your main goal: JSDOMWindow::getOwnPropertyDescriptor. I'd recommend leaving it for later.

[Kent Hansen]

(In reply to comment #11)
> > I think JSCallbackObject should reimplement getOwnPropertyDescriptor()
> 
> Me too.
> 
> But let's separate out two concerns.
> 
> Concern 1: Switching hasOwnProperty to use getOwnPropertyDescriptor would break
> hasOwnProperty for API objects.
> 
> Concern 2: getOwnPropertyDescriptor has never worked very well for API objects.
> 
> Concern 1 is easy to solve: Have JSCallbackObject::getOwnPropertyDescriptor
> call JSCallbackObject:: getOwnPropertySlot and return a value descriptor, and
> add a hasOwnProperty and getOwnPropertyDescriptor test to testapi.c. That
> maintains current behavior for hasOwnProperty. It also solves a bit of Concern
> 2, because getOwnPropertyDescriptor will stop returning 'undefined' for API
> properties.
> 
> I think your more ambitious proposal for solving Concern 2 sounds reasonable,
> but Concern 2 is a sidetrack from your main goal:
> JSDOMWindow::getOwnPropertyDescriptor. I'd recommend leaving it for later.

Sounds like a plan to me, thanks!