366292 - __defineSetter__("x", ...) changes x's value to the string "__lookupSetter__"

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Jesse Ruderman]

js> this.__defineSetter__("x", function(){}); x;
__lookupSetter__

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

When I try this, I see:
js> this.__defineSetter__("x", function(){}); x;
Assertion failure: (sprop)->slot != SPROP_INVALID_SLOT, at jsinterp.c:4245

The assertion (and related code) isn't accounting for JSPROP_SHARED properties.

Comment 2

[Brendan Eich [:brendan]]

Attachment: fix

A regression from the patch for bug 365851.

Igor, feel free to review too.

/be

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Does NATIVE_SET need similar treatment?

Comment 4

[Brendan Eich [:brendan]]

(In reply to comment #3)
> Does NATIVE_SET need similar treatment?

No, because there's nowhere for the value to go with a stub setter and no slot. Note how SPROP_SET raises an error if you try to set a property that has only a getter.  Some time ago (ECMA Ed. 3 time, but getters and setters didn't make it into that spec), the thinking apparently was that it's better to have an error than a black hole.  See rev 3.9 of jsscope.h (no bug for this change).

Note how the reverse case, the one this bug ran into, works: you can get undefined from a property that has only a setter.  This is being spec'ed for ES4, so shout now if it seems wrong.

/be

Comment 5

[Brendan Eich [:brendan]]

Fixed on trunk:

js/src/jsinterp.c rev 3.319

/be

Comment 7

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/js1_5/Regress/regress-366292.js,v  <--  regress-366292.js
initial revision: 1.1

Comment 8

[Daniel Veditz [:dveditz]]

Needed on branches as a prerequisite to fix bug 367501?

Comment 9

[Brendan Eich [:brendan]]

Comment on attachment 250879 [details] [diff] [review]
fix

Sorry I missed this -- it must go into the branches.

/be

Comment 10

[Daniel Veditz [:dveditz]]

Comment on attachment 250879 [details] [diff] [review]
fix

approved for 1.8/1.8.0 branches, a=dveditz for drivers

Comment 11

[Brendan Eich [:brendan]]

Comment on attachment 250879 [details] [diff] [review]
fix

Patch applies (cleanly to 1.8 branch; for 1.8.0 branch with fuzz of 10, I didn't try to minimize that parameter) to both branches.

/be

Comment 12

[moz_bug_r_a4]

Besides NATIVE_GET, NATIVE_SET needs to be fixed on branches.

On trunk, NATIVE_SET seems to be no longer used.
http://lxr.mozilla.org/mozilla/search?string=NATIVE_SET

Comment 13

[Brendan Eich [:brendan]]

Argh, thanks moz_bug_r_a4.  I'm on it.

/be

Comment 14

[Brendan Eich [:brendan]]

Attachment: followup fix for 1.8 branch

I'm half-inclined to just check this in, but want mrbkap to stamp (moz_bug_r_a4 please feel free to test and review too).

/be

Comment 15

[Brendan Eich [:brendan]]

Attachment: followup fix for the 1.8.0 branch

Comment 16

[Daniel Veditz [:dveditz]]

Comment on attachment 252116 [details] [diff] [review]
followup fix for 1.8 branch

a=dveditz for 1.8 branch

Comment 17

[Daniel Veditz [:dveditz]]

Comment on attachment 252118 [details] [diff] [review]
followup fix for the 1.8.0 branch

a=dveditz for 1.8.0 branch

Comment 18

[Brendan Eich [:brendan]]

Fixed:

1.8:   js/src/jsinterp.c 3.181.2.82
1.8.0: js/src/jsinterp.c 3.181.2.17.2.27

/be

Comment 19

[Bob Clary [:bc] (inactive)]

verified fixed 1.8.0, 1.8.1, 1.9.0 2007-01-23 win/mac*/linux