197940 - JS setter in prototype can't be overridden in delegating objects

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Brendan Eich [:brendan]]

Old-ish bug, from when I over-eagerly tested for both JSPROP_SHARED (which is
exactly the attribute that means don't-override in this case) and JSPROP_SETTER
in js_SetProperty.  Patch coming up.

/be

Comment 1

[Brendan Eich [:brendan]]

Attachment: one-line fix, woohoo!

I lxr'd for JSPROP_SETTER and the few places outside of js/src that use it,
also use JSPROP_SHARED appropriately.

/be

Comment 2

[rogerl (gone)]

Comment on attachment 117560 [details] [diff] [review]
one-line fix, woohoo!

r=rogerl

Comment 3

[Brendan Eich [:brendan]]

I checked the patch in.  However, the patch isn't going to help the case that
Georg cares about.  I dug deeper into CVS history and found that for bug 130970,
as a memory-saving "bloat reduction sneak" change, not needed for the main fix
in that bug's final patch, I added JSPROP_SHARED to all getters and setters
defined by script.

I did that purely for space-saving and anti-garbage-entrainment reasons.  But it
seems that change also restored forward compatibility with ECMA Edition 4 / JS2.

The patch in this bug, attachment 117560 [details] [diff] [review], removes the JSPROP_SETTER bit from the
bitset tested by js_SetProperty to decide whether to defeat overriding
("shadowing") of a prototype property.  That's well and good, and that change
has been made.  It helps native implementations that define unshared setters and
then override them via JS_SetProperty or assignment in a script.  However, it's
not enough to help Georg's case of script-defined getters and setters, because
all script-defined getters and setters still are defined with JSPROP_SHARED.

Again, using the C JS API (jsapi.h), native object implementations are free to
create unshared, slot-full getters and setters.  But users of the language
cannot -- user-defined scripts can create only shared, slot-less getters and
setters.  This is forward-compatible with ECMA Edition 4 (JS2), I believe, but
I'd like waldemar to weigh in.

This bug's URL refers to Georg's news posting containing the testcase.  Here is
a commentary on the testcase inputs and outputs (my comments use // notation and
follow the line they're commenting on):

js> function A(){}
js> function B(){this.__defineGetter__('a',function(){return false;})}
js> A.prototype=new B;
[object Object]
js> a=new A;
[object Object]
js> a.a=6;
5: TypeError: setting a property that has only a getter

// This error was added a while ago for Edition 4 forward compatibility.

js> delete a.a;
true

// Since a.a delegates to a's prototype, delete does nothing and returns true,
// as expected.

js> a.a=6;
7: TypeError: setting a property that has only a getter

// See above.

js> a.a
false

// This invokes the getter, with expected result.

js> a.hasOwnProperty('a');
true 

// I do not see this result with current SpiderMonkey sources -- I see false
// instead.  Georg, what Mozilla version did you use to get this result from
// a.hasOwnProperty('a')?

What I see when I run this test in the current JS shell is consistent with
SpiderMonkey's implementation rules, and with what I understand to be ECMA
Edition 4 / JS2 semantics.

The objection in Georg's post was that neither delete nor assignment seemed able
to override a getter inherited from a prototype.  One bug in the testcase was
the lack of a setter.  It's an error to assign to an id that has only a getter.
If we fix that with a setter that returns false, like so:

js> A.prototype.__defineSetter__('a',function(){return false})
js> a.a
false
js> a.a=4
false
js> a.a
false
js> a.hasOwnProperty('a')
false

Then we see that it is indeed not possible to override using assignment (or
delete, but I don't show that futile delete again).  But it is possible to
override the prototype's getter by defining a getter on the delegating object:

js> a.__defineGetter__('a',function(){return true})
js> a.a
true
js> a.hasOwnProperty('a')
true

So you can override a getter inherited from a prototype, but only with another
getter; likewise for setter.  Is this also forward-compatible with Edition 4?  I
think it is, but I was reading the proposed semantics quickly.  Waldemar can
speak with authority.
 
/be

Comment 4

[Brendan Eich [:brendan]]

I talked to waldemar.  Although JS2 / ES4 has no __defineSetter__, if it did,
the behavior with regard to shadowing a proto-setter vs. calling it with this
bound to the delegating instance could "go either way."  For the case of JS2
instance setters, a superclass's setter will be called on a subclass instance
that has no overriding instance setter, and that's what SpiderMonkey emulates
most closely now.

So to override a prototype setter, you have to define a setter of the same id in
the delegating instance.  The delete operator finds nothing in the delegating
instance to remove, and an assignment operator calls the proto-setter.

Although the testcase was invalid, and the request for delete and assignment to
have a different effect was also not valid (because not forward compatible and
less dynamic-memory-efficient in SpiderMonkey), I'm marking this bug fixed. The
jsobj.c change did fix a small logic bug in js_SetProperty, one that could be
detected by native object implementations.

/be

Comment 5

[Phil Schwartau]

Marking verified. This checkin has caused no test regressions
in the JS testsuite when applied on WinNT, Linux, and Mac OSX.

In the current JS shell, I get the same results as Brendan did
in Comment #3 -

Comment 6

[[e-mail]]

I don't know if it's relevant, but there's a "band-aid" fix to autocomplete when
XBL attaches a prototype with a getter/setter to an element that may already
have a a standard property with the same name, the "band-aid" uses delete to
ensure that the getter/setter are used, I assume that nothing here affects that.

Comment 7

[Matthias Versen [:Matti]]

Comment on attachment 117560 [details] [diff] [review]
one-line fix, woohoo!

r=rogerl via comment#2