547087 - Holes in implementation of undefined/NaN/Infinity as non-writable/non-configurable

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

[jwalden@the-great-waldo-search src]$ dbg/js -j
js> undefined = 17;
17
js> Object.getOwnPropertyDescriptor(this, "undefined")
({value:17, writable:true, enumerable:true, configurable:true})

The resolve hook's short-circuiting because it's an assigning case, and it's assumed the property has the normal attributes (writable, configurable, enumerable) so defining a different property is the same as modifying the existing one.  This also appears to apply to NaN and Infinity as well.


[jwalden@the-great-waldo-search src]$ dbg/js -j
js> for (var p in this);
js> Object.getOwnPropertyDescriptor(this, "undefined")
({value:(void 0), writable:true, enumerable:false, configurable:false})

I think JS_EnumerateStandardClasses needs an update for this, going by inspection.


These likely need beta-testing for real-world impact.

Comment 1

[Brendan Eich [:brendan]]

Just for my own sanity: two bugs, one in global_resolve (old one, mrbkap and I did it long ago), one in JS_EnumerateStandardClasses (not changed when other undefined-defining code changed for bug 537863). Second bug suggests (dare I say it) adding a common DefineUndefined helper ;-).

/be

Comment 2

[Brendan Eich [:brendan]]

Talking on IRC, it looks like nsWindowSH::NewResolve does not have the bug that js/src/shell/js.cpp's global_resolve has.

/be

Comment 3

[Jeff Walden [:Waldo]]

Attachment: Partial patch, to get it out of my mq

Here's the easy part, JS_EnumerateStandardClasses.  |global_resolve| is a tougher nut that I'd like to defer possibly until bug 547140 is considered -- would like to get this out of my mq and not let perfect be the enemy of good for now.

Comment 4

[Jason Orendorff [:jorendorff]]

Comment on attachment 429034 [details] [diff] [review]
Partial patch, to get it out of my mq

OK.

I would have preferred fixing global_resolve too, if it's just a matter of removing the if-guard. (I'm gung-ho about removing JSRESOLVE_ASSIGNING too, don't get me wrong...)

Comment 5

[Brendan Eich [:brendan]]

What's the problem with patching global_resolve?

/be

Comment 6

[Jeff Walden [:Waldo]]

http://hg.mozilla.org/tracemonkey/rev/090b914be39b

Simply removing the check means you get the extra work of enumerating standard classes for any unrecognized name, which doesn't seem quite like a performance win, and seems like it might be avoidable.  Maybe it's not, but it's still more amenable to being fixed in a global solution in bug 547140.

Comment 7

[Brendan Eich [:brendan]]

(In reply to comment #6)
> http://hg.mozilla.org/tracemonkey/rev/090b914be39b
> 
> Simply removing the check means you get the extra work of enumerating standard
> classes for any unrecognized name,

Unrecognized name hitting global_resolve is either an Object.prototype property (rare, don't care) or a ReferenceError aborning (definitely don't care).

> which doesn't seem quite like a performance win,

We need to win the game that counts, which is neither

js> toString()
"[object global]"

nor

js> arflbarfl
typein:1: ReferenceError: arflbarfl is not defined

/be

Comment 8

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/090b914be39b