Bug 33948 - Object.getOwnPropertyDescriptor(window) returns descriptors for properties in the prototype chain
: Object.getOwnPropertyDescriptor(window) returns descriptors for properties in...
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript
: 528+ (Nightly build)
: PC Mac OS X 10.5
: P2 Normal
Assigned To: Kent Hansen
:
Depends on:
Blocks: 33947
  Show dependency treegraph
 
Reported: 2010-01-21 06:42 PST by Kent Hansen
Modified: 2010-01-26 12:07 PST (History)
3 users (show)

See Also:


Attachments
Proposed patch (9.20 KB, patch)
2010-01-21 09:48 PST, Kent Hansen
oliver: review-
oliver: commit‑queue-
Details | Formatted Diff | Diff
Updated patch (more elaborate test) (119.16 KB, patch)
2010-01-22 03:30 PST, Kent Hansen
no flags 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-21 06:42:43 PST
For example, addEventListener is a property of the window prototype object, yet Object.getOwnPropertyDescriptor(window, "addEventListener") returns a descriptor for it.
JSDOMWindow::getOwnPropertyDescriptor() should not return the descriptor for properties in the prototype.
Comment 1 Kent Hansen 2010-01-21 09:48:12 PST
Created attachment 47129 [details]
Proposed patch
Comment 2 Darin Adler 2010-01-21 15:18:49 PST
Comment on attachment 47129 [details]
Proposed patch

> +    var names = Object.getOwnPropertyNames(o);
> +    for (var i = 0; i < names.length; ++i)
> +        propertySet[names[i]] = true;

When the test is written this way, the results are unpredictable because the order property names come out of getOwnPropertyNames is unpredictable and depends on hash table ordering. This creates a fragile test with results that get shuffled around any time someone adds or removes a property or changes the hash algorithm. Instead I suggest we have a test that sorts the names so things are in a predictable order.

See tests like window-properties.html for an example of how this has been done in the past.
Comment 3 Oliver Hunt 2010-01-21 16:53:02 PST
Comment on attachment 47129 [details]
Proposed patch

r=me for the code, but your test is liekly to be unstable, possibly just do a simple sort on the output of getOwnPropertyNames?  I think additionally your test should test to ensure that you still see own properties.
Comment 4 Kent Hansen 2010-01-22 01:20:11 PST
(In reply to comment #3)
> (From update of attachment 47129 [details])
> r=me for the code, but your test is liekly to be unstable, possibly just do a
> simple sort on the output of getOwnPropertyNames?  I think additionally your
> test should test to ensure that you still see own properties.

Agreed on the sorting, I didn't think of it until I was on the subway home.
I did set out to write a "positive" test that uses getOwnPropertyNames() and verifies that those properties have descriptors, but then I'll need a property exclusion list similar to the one in window-properties.html to keep the test robust, and I'd prefer not to copy&paste that code or have to do test refactoring in this change (I have a different task for refactoring those tests already, https://bugs.webkit.org/show_bug.cgi?id=32596).
Well, I could create a task "Add getOwnPropertyDescriptor test for window object" and make it a blocker for this task.
Comment 5 Kent Hansen 2010-01-22 03:30:48 PST
Created attachment 47187 [details]
Updated patch (more elaborate test)

Test properties that should have descriptors as well as those that should not.
Comment 6 WebKit Commit Bot 2010-01-22 11:09:56 PST
Comment on attachment 47187 [details]
Updated patch (more elaborate test)

Clearing flags on attachment: 47187

Committed r53706: <http://trac.webkit.org/changeset/53706>
Comment 7 WebKit Commit Bot 2010-01-22 11:10:01 PST
All reviewed patches have been landed.  Closing bug.
Comment 9 Eric Seidel 2010-01-22 17:06:02 PST
Looks like there was an attempt to fix this in r53738
Comment 10 Kent Hansen 2010-01-25 01:55:40 PST
(In reply to comment #9)
> Looks like there was an attempt to fix this in r53738

Fix is good, sorry for the breakage.
Adding "debug vs release" to my Things-to-keep-in-mind-before-submitting-tests checklist. :)
Comment 11 Oliver Hunt 2010-01-25 01:59:20 PST
Has this been rolled out? there aren't comments to suggest that it was, but it's also been reopened implying it was rolled out.
Comment 12 Kent Hansen 2010-01-25 02:03:53 PST
(In reply to comment #11)
> Has this been rolled out? there aren't comments to suggest that it was, but
> it's also been reopened implying it was rolled out.

Nope.
Comment 13 Kent Hansen 2010-01-26 07:35:11 PST
The tests aren't failing anymore (the fix Eric pointed to also fixed Windows Debug). Can we close this task again?
Comment 14 Eric Seidel 2010-01-26 12:07:36 PST
I don't see any reason to keep it open.