Bug 33603 - Some window properties are not returned by Object.keys and Object.getOwnPropertyNames
Summary: Some window properties are not returned by Object.keys and Object.getOwnPrope...
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Kent Hansen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-01-13 08:56 PST by Kent Hansen
Modified: 2010-01-21 01:19 PST (History)
2 users (show)

See Also:


Attachments
Proposed patch (18.62 KB, patch)
2010-01-19 08:38 PST, Kent Hansen
ggaren: review-
kent.hansen: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kent Hansen 2010-01-13 08:56:31 PST
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.
Comment 1 Kent Hansen 2010-01-19 08:38:16 PST
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.
Comment 2 Darin Adler 2010-01-19 08:46:30 PST
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.
Comment 3 Kent Hansen 2010-01-19 09:20:46 PST
(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.
Comment 4 Darin Adler 2010-01-19 09:22:59 PST
The people I’d ask about this are Geoff Garen, Sam Weinig, and Maciej Stachowiak.
Comment 5 Darin Adler 2010-01-19 09:23:07 PST
And Oliver Hunt.
Comment 6 Geoffrey Garen 2010-01-19 12:59:41 PST
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".
Comment 7 Geoffrey Garen 2010-01-19 13:02:15 PST
> 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".
Comment 8 Kent Hansen 2010-01-20 02:08:29 PST
(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.
Comment 9 Kent Hansen 2010-01-20 04:43:42 PST
(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 :) ).
Comment 10 Kent Hansen 2010-01-20 09:20:05 PST
(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.
Comment 11 Geoffrey Garen 2010-01-20 11:53:41 PST
> 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.
Comment 12 Kent Hansen 2010-01-21 01:19:16 PST
(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!