|Summary:||Object.getOwnPropertyDescriptor(window) returns descriptors for properties in the prototype chain|
|Product:||WebKit||Reporter:||Kent Hansen <kent.hansen>|
|Severity:||Normal||CC:||commit-queue, eric, oliver|
|Version:||528+ (Nightly build)|
|OS:||OS X 10.5|
|Bug Depends on:|
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 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 8 Eric Seidel (no email) 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 Eric Seidel (no email) 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 (no email) 2010-01-26 12:07:36 PST
I don't see any reason to keep it open.