Closed
Bug 537863
Opened 15 years ago
Closed 15 years ago
Make various global properties (NaN, Infinity, undefined) readonly per ES5
Categories
(Core :: JavaScript Engine, defect, P1)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla1.9.3a1
People
(Reporter: Waldo, Assigned: Waldo)
References
Details
(Keywords: dev-doc-complete, Whiteboard: fixed-in-tracemonkey)
Attachments
(2 files)
(deleted),
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
ES5 requires it, easy to change, should do this yesterday for long long bake time before the next release.
Assignee | ||
Comment 1•15 years ago
|
||
Patch in hand, process of writing tests reminded me that |obj.readonly = 17| doesn't throw in strict mode, and I see not to have it in any of my patch queues waiting to be pushed or reviewed. Filed bug 537873, know what's wrong, will fix that first but post this bug's patch for review so that we make progress here even if that one remains temporarily.
More incompatible changes to fix ASAP and shout to high heaven -- although, at least with this one, it doesn't have tangential effects.
Attachment #420034 -
Flags: review?(jorendorff)
Updated•15 years ago
|
Attachment #420034 -
Flags: review?(jorendorff) → review+
Assignee | ||
Comment 2•15 years ago
|
||
http://hg.mozilla.org/tracemonkey/rev/e0b4bf47c653
Will do some bloggeration about this after it hits mozilla-central...
Keywords: dev-doc-needed
Whiteboard: fixed-in-tracemonkey
Assignee | ||
Comment 3•15 years ago
|
||
One of the Mochitests has |case undefined| in a switch that we optimize into a lookupswitch. In jsops.cpp for JSOP_LOOKUPSWITCH, we bail immediately if the incoming value isn't a string or a number or a boolean right now. However, when emitting bytecode we optimize case-expression lookups when the name is guaranteed to refer to a readonly, permanent property whose value is primitive -- which includes |undefined|. This mismatch, never before visible because we had no global immutable properties whose value was |undefined|, resulted in |case undefined| being impossible to hit in any switch that could be optimized into a lookupswitch. This patch loosens the is-boolean check in the guard in lookupswitch into an is-special check so that we guard on all possible value types that might be in the lookup table.
I went ahead and pushed this with an anticipatory r=jorendorff -- it's a small fix that's reasonably simple to understand when explained, and I don't really feel like backing out the patch to make the properties immutable. Let me know if you think there's something better to do here. (I do think we want to constify here, so the only other fix I saw -- equivalently narrowing in EmitSwitch in jsemit.cpp -- seemed a bad idea.)
Attachment #420490 -
Flags: review?(jorendorff)
Comment 4•15 years ago
|
||
Fine to make progress.
One issue: js_AtomizePrimitiveValue handles null as well as undefined, so the interpreter should too. It does not handle all specials, though, asserting only that v is one of JSVAL_IS_{STRING,DOUBLE,INT,BOOLEAN,NULL,VOID}. Ugh, but we want jsops.cpp JSOP_LOOKUPSWITCH code to be faster than a four or five way JSVAL_IS_FOO || chain.
To future proof and make consistent, how about just testing JSVAL_IS_PRIMITIVE in jsops.cpp? That still takes in all specials, but the assertion in jsatom.cpp will keep us from getting to runtime, during codegen.
/be
Assignee | ||
Comment 5•15 years ago
|
||
http://hg.mozilla.org/tracemonkey/rev/ad40bcea2f5a
Good call. Although, |case null| doesn't actually get hit by lookupswitch because it deoptimizes to condswitch -- filed {{bug|538482}} on that.
Also, unrelated, I had to land this change to adjust a test that assigned to undefined to assign to a different variable (that's hopefully like enough to undefined as it was before to trigger the same bug conditions):
http://hg.mozilla.org/tracemonkey/rev/24083cc5953d
Assignee | ||
Comment 6•15 years ago
|
||
I have clearly been spending too much time in wikis lately.
Updated•15 years ago
|
Attachment #420490 -
Flags: review?(jorendorff) → review+
Comment 7•15 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Comment 8•15 years ago
|
||
waldo, ecma_5/misc/jstests.list needs to be listed in ecma_5/jstests.list
Comment 9•15 years ago
|
||
fixed manifest http://hg.mozilla.org/tracemonkey/rev/3970759ca2c1
Comment 10•14 years ago
|
||
Updated:
https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/Infinity
https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/undefined
https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/NaN
Also mentioned on:
https://developer.mozilla.org/en/JavaScript/New_in_JavaScript/1.8.5
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•