Bug 33948 – Object.getOwnPropertyDescriptor(window) returns descriptors for properties in the prototype chain

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Kent Hansen]

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.

[Kent Hansen]

Created attachment 47129 [details]
Proposed patch

[Darin Adler]

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.

[Oliver Hunt]

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.

[Kent Hansen]

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

[Kent Hansen]

Created attachment 47187 [details]
Updated patch (more elaborate test)

Test properties that should have descriptors as well as those that should not.

[WebKit Commit Bot]

Comment on attachment 47187 [details]
Updated patch (more elaborate test)

Clearing flags on attachment: 47187

Committed r53706: <http://trac.webkit.org/changeset/53706>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

Looks like there was an attempt to fix this in r53738

[Kent Hansen]

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

[Oliver Hunt]

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.

[Kent Hansen]

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

[Kent Hansen]

The tests aren't failing anymore (the fix Eric pointed to also fixed Windows Debug). Can we close this task again?

[Eric Seidel (no email)]

I don't see any reason to keep it open.