Closed Bug 537863 Opened 10 years ago Closed 10 years ago

Make various global properties (NaN, Infinity, undefined) readonly per ES5

Categories

(Core :: JavaScript Engine, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla1.9.3a1

People

(Reporter: Waldo, Assigned: Waldo)

References

Details

(Keywords: dev-doc-complete, Whiteboard: fixed-in-tracemonkey)

Attachments

(2 files)

ES5 requires it, easy to change, should do this yesterday for long long bake time before the next release.
Depends on: 537873
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)
Attachment #420034 - Flags: review?(jorendorff) → review+
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
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)
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
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
I have clearly been spending too much time in wikis lately.
Attachment #420490 - Flags: review?(jorendorff) → review+
http://hg.mozilla.org/mozilla-central/rev/e0b4bf47c653
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
waldo, ecma_5/misc/jstests.list needs to be listed in ecma_5/jstests.list
Depends on: 547087
No longer depends on: 537873
Depends on: 537873
You need to log in before you can comment on or make changes to this bug.