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


Attachments
Proposed patch (9.20 KB, patch)
2010-01-21 09:48 PST, Kent Hansen
oliver: review-
oliver: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Updated patch (more elaborate test) (119.16 KB, patch)
2010-01-22 03:30 PST, Kent Hansen
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 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 From 2010-01-21 09:48:12 PST -------
Created an attachment (id=47129) [details]
Proposed patch
------- Comment #2 From 2010-01-21 15:18:49 PST -------
(From update of attachment 47129 [details])
> +    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 From 2010-01-21 16:53:02 PST -------
(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.
------- Comment #4 From 2010-01-22 01:20:11 PST -------
(In reply to comment #3)
> (From update of attachment 47129 [details] [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 From 2010-01-22 03:30:48 PST -------
Created an attachment (id=47187) [details]
Updated patch (more elaborate test)

Test properties that should have descriptors as well as those that should not.
------- Comment #6 From 2010-01-22 11:09:56 PST -------
(From update of attachment 47187 [details])
Clearing flags on attachment: 47187

Committed r53706: <http://trac.webkit.org/changeset/53706>
------- Comment #7 From 2010-01-22 11:10:01 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #8 From 2010-01-22 17:02:50 PST -------
This broke Leopard:
http://build.webkit.org/results/Leopard%20Intel%20Debug%20(Tests)/r53706%20(9644)/fast/dom/Window/window-property-descriptors-pretty-diff.html

Looks like it needs new baselines.
------- Comment #9 From 2010-01-22 17:06:02 PST -------
Looks like there was an attempt to fix this in r53738
------- Comment #10 From 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 From 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 From 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 From 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 From 2010-01-26 12:07:36 PST -------
I don't see any reason to keep it open.