| Summary: | Error properties of the Global Object are missing the DontEnum attribute | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Kent Hansen <kent.hansen> | ||||||||
| Component: | JavaScriptCore | Assignee: | Kent Hansen <kent.hansen> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | commit-queue, eric, hausmann, vestbo, webkit.review.bot | ||||||||
| Priority: | P2 | ||||||||||
| Version: | 528+ (Nightly build) | ||||||||||
| Hardware: | PC | ||||||||||
| OS: | Linux | ||||||||||
| Bug Depends on: | 42258, 42354 | ||||||||||
| Bug Blocks: | |||||||||||
| Attachments: |
|
||||||||||
Created attachment 38666 [details]
Proposed patch
Comment on attachment 38666 [details]
Proposed patch
Great, but this needs a ChangeLog and a test. I expect that this will already affect other existing tests, and might not need a new one. But new one for specifically testing this might still be a good idea.
Created attachment 44310 [details]
Revised patch (adds ChangeLog and test)
style-queue ran check-webkit-style on attachment 44310 [details] without any errors.
Comment on attachment 44310 [details]
Revised patch (adds ChangeLog and test)
cq+ on this, too. Kent, you may want to set cq? for your patches :)
Comment on attachment 44310 [details]
Revised patch (adds ChangeLog and test)
Rejecting patch 44310 from commit-queue.
Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1']" exit_code: 1
Running build-dumprendertree
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 11730 test cases.
fast/dom/prototype-inheritance.html -> failed
Exiting early after 1 failures. 5574 tests run.
81.77s total testing time
5573 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
1 test case (<1%) had stderr output
Looks like this patch didn't update all the needed tests. (In reply to comment #7) > Looks like this patch didn't update all the needed tests. Right, fast/dom/prototype-inheritance.html relies on these properties to be returned by for..in. One way to "fix" that is to remove the *Error properties from the expected test data. A different solution would be to add the set of built-in ECMA properties (that are known to be non-enumerable but present) to the set of properties being tested, thus keeping what's being tested but making the test more robust. Second option sounds obvious to me, unless there's a particular reason for that test only testing enumerable properties. We could have an explicit list of objects instead of crawling around the DOM. Ideally such a list would be in a central place so it could be shared by all these scripts. In a perfect world it would be possible to update such a list automatically from the idls. (In reply to comment #9) > We could have an explicit list of objects instead of crawling around the DOM. > Ideally such a list would be in a central place so it could be shared by all > these scripts. In a perfect world it would be possible to update such a list > automatically from the idls. Spun off as https://bugs.webkit.org/show_bug.cgi?id=32596 and made it a blocker for this bug. Even if that's fixed, the current patch for this bug should be changed to r-, since there's another test I need to update (one that actually tests enumerability, in which case it's correct to remove the *Error properties from the expected test output). Comment on attachment 44310 [details]
Revised patch (adds ChangeLog and test)
Marking r- per Kent's comment.
Removing dependency on 32596; after reviewing the current status I've decided it's better to descope that work and focus on improving individual tests first. Created attachment 61643 [details] Patch Requires that the fixes for https://bugs.webkit.org/show_bug.cgi?id=42258 and https://bugs.webkit.org/show_bug.cgi?id=42354 are landed first, otherwise the relevant tests would regress due to these properties no longer appearing in for..in enumeration. Comment on attachment 61643 [details]
Patch
r-. Since you are changing the DontEnum status of 6 properties, we should really have tests that test all 6 properties. The existing tests only cover EvalError.
(In reply to comment #14) > (From update of attachment 61643 [details]) > r-. Since you are changing the DontEnum status of 6 properties, we should really have tests that test all 6 properties. The existing tests only cover EvalError. No, the sputnik tests cover all 6 properties, but they abort as soon as one test fails (and EvalError happens to be the first property that's tested). Have a look at e.g. LayoutTests/fast/js/sputnik/Conformance/10_Execution_Contexts/10.1_Definitions/10.1.5_Global_Object/S10.1.5_A2.1_T3.html, line 95 and onward. Comment on attachment 61643 [details]
Patch
Setting r? again since the existing tests do cover all relevant properties, as described in the previous comment.
Comment on attachment 61643 [details]
Patch
looks ok.
Committed r63882: <http://trac.webkit.org/changeset/63882> |
The properties "EvalError", "RangeError", "ReferenceError", "SyntaxError", "TypeError", "URIError" don't have the DontEnum attribute, and consequently they show up in a for..in statement. According to ECMA-262, chapter 15, these properties should not be enumerable ("Every other property described in this section has the attribute { DontEnum } (and no others) unless otherwise specified"). Curiously, the "Error" property does have the attribute, but the more specific errors do not.