Bug 28771 – Error properties of the Global Object are missing the DontEnum attribute

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Kent Hansen]

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.

[Kent Hansen]

Created attachment 38666 [details]
Proposed patch

[Eric Seidel (no email)]

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.

[Kent Hansen]

Created attachment 44310 [details]
Revised patch (adds ChangeLog and test)

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44310 [details] without any errors.

[Simon Hausmann]

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

[WebKit Commit Bot]

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

[Eric Seidel (no email)]

Looks like this patch didn't update all the needed tests.

[Kent Hansen]

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

[Eric Seidel (no email)]

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.

[Kent Hansen]

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

[Eric Seidel (no email)]

Comment on attachment 44310 [details]
Revised patch (adds ChangeLog and test)

Marking r- per Kent's comment.

[Kent Hansen]

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.

[Kent Hansen]

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.

[Sam Weinig]

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.

[Kent Hansen]

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

[Kent Hansen]

Comment on attachment 61643 [details]
Patch

Setting r? again since the existing tests do cover all relevant properties, as described in the previous comment.

[Kent Tamura]

Comment on attachment 61643 [details]
Patch

looks ok.

[Kent Hansen]

Committed r63882: <http://trac.webkit.org/changeset/63882>