Closed Bug 523846 Opened 10 years ago Closed 10 years ago

Back out bug 478047 and revert to past behavior when setting a property that only has a getter

Categories

(Core :: JavaScript Engine, defect)

Other Branch
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
status1.9.2 --- final-fixed

People

(Reporter: jorendorff, Assigned: Waldo)

References

Details

(Keywords: regression, testcase, Whiteboard: fixed-in-tracemonkey)

Attachments

(2 files, 1 obsolete file)

var o = { get p() { return "a"; } };
o.p = "b";  // [1]
assertEq(o.p, "a");

function T() {}
T.prototype = o;
y = new T;
y.p = "b";  // [2]
assertEq(y.p, "a");

From what I can tell looking at the April 2009 draft of ES5, assignments [1] and [2] must pass silently in non-strict mode, and both must throw a TypeError in strict mode.

Per 11.2.1, `o.p` evaluates to a Reference with base value=o, referenced
name="p", and strict flag=strictness of the surrounding code.

Per 11.13.1, `o.p = "b"` calls PutValue(V, "b"), where V is the above
reference.

Per 8.7.2, PutValue ends up in step 4.b calling the [[ThrowingPut]] internal
method of o, passing "p", "b", and the strict flag from above.

Per 8.12.5, step 1, the [[ThrowingPut]] method only throws a TypeError if the
Throw argument is true.

(I checked that this is indeed a case that causes [[CanPut]] to return false.)
Nominating because this is a regression that might break web pages.
Blocks: 478047
Flags: blocking1.9.2?
mmm, yes. we might find some with the beta, even!
Flags: blocking1.9.2? → blocking1.9.2+
Confirmed this bug is in 1.9.2.

It only affects pages that use getters. It's not specific to the magic `{ get p() ... }` syntax, though; it affects code that uses __defineGetter__ too.
Flags: blocking1.9.2+ → blocking1.9.2?
yes, still a blocker. :-)
Flags: blocking1.9.2? → blocking1.9.2+
Assignee: general → jwalden+bmo
Status: NEW → ASSIGNED
I would appreciate a setting giving me the facility to force exceptions even in non strict mode for any read only stuff, because each application attempting to assign a value to something that is read only (i.e. because declared as const or because it has a getter only but no setter) is broken, independent of whether an error is reported or not. So throwing an exception helps to detect broken applications and also helps to identify the reasons, why it is broken. So if exception throwing in this case is disabled by default for non strict mode, I would appreciate a setting giving me the facility to force the exceptions even in non strict mode.
We don't have strict mode yet, so some of the testing is flipped off, but I think the rest of it is sound.  Note that I preserved behavior of js_GetterOnlyPropertyStub as that function is distinct from what ES5 requires -- WebIDL might have something to say about that, but for now I think we should leave its behavior as-is.
Attachment #411583 - Flags: review?(jorendorff)
Hm, let's shift this, jorendorff is busy with a Baedeker for a bug.

I don't think it's worth preserving any reasoned behavior for the 414098 test changes (ES5 sez it's implementation defined), so I just copied actual behavior to what was expected.
Attachment #411583 - Attachment is obsolete: true
Attachment #411592 - Flags: review?(brendan)
Attachment #411583 - Flags: review?(jorendorff)
Comment on attachment 411592 [details] [diff] [review]
...and with existing tests fixed or removed, as appropriate

> JS_FRIEND_API(JSBool)
> js_GetterOnlyPropertyStub(JSContext *cx, JSObject *obj, jsval id, jsval *vp)

Seems this is dead -- remove it and no need to patch it.

>@@ -691,10 +691,8 @@ JSScopeProperty::set(JSContext* cx, JSOb
>         return js_InternalGetOrSet(cx, obj, id, fval, JSACC_WRITE, 1, vp, vp);
>     }
> 
>-    if (attrs & JSPROP_GETTER) {
>-        js_ReportGetterOnlyAssignment(cx);
>-        return false;
>-    }
>+    if (attrs & JSPROP_GETTER)
>+        return js_ReportGetterOnlyAssignment(cx) == JS_TRUE;

I grepped to confirm: !! is preferred to convert JSBool to bool.

r=me with these addressed.

This is gonna need a tweak to use JSREPORT_STRICT_MODE_ERROR when that lands, right? Need another followup?

/be
Attachment #411592 - Flags: review?(brendan) → review+
(In reply to comment #8)
> Seems this is dead -- remove it and no need to patch it.

Have to grep outside js/src to find its use.

> This is gonna need a tweak to use JSREPORT_STRICT_MODE_ERROR when that lands,
> right? Need another followup?

Yeah.

Landed and backed out -- turns out we have Mochitests that depend on assignment to a getter-only property failing:

23976 ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/test_classList.html | assigning to classList didn't throw
24329 ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/test_classList.html | assigning to classList didn't throw
24682 ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/test_classList.html | assigning to classList didn't throw
25035 ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/test_classList.html | assigning to classList didn't throw
40288 ERROR TEST-UNEXPECTED-FAIL | /tests/content/media/test/test_networkState.html | Should not be able to set networkState (readonly attribute)
41075 ERROR TEST-UNEXPECTED-FAIL | /tests/content/media/test/test_readyState.html | Should not be able to set readyState (readonly attribute)
41079 ERROR TEST-UNEXPECTED-FAIL | /tests/content/media/test/test_seek.html | r11025_s16_c1.wav seek test 1: seeking should be readonly
41102 ERROR TEST-UNEXPECTED-FAIL | /tests/content/media/test/test_seek.html | seek.ogv seek test 1: seeking should be readonly
41125 ERROR TEST-UNEXPECTED-FAIL | /tests/content/media/test/test_seek.html | 320x240.ogv seek test 1: seeking should be readonly
45220 ERROR TEST-UNEXPECTED-FAIL | /tests/content/xbl/test/test_bug371724.xhtml | Setting a readonly xbl property should throw a TypeError exception

I can correct all those to check for the original value after the silently-failing set, but are we sure we want to do that now?  I'm thinking it may make more sense to revert to the previous behavior and then switch to this super-early in the 3.7 cycle.

Thoughts?
I agree.
Attachment #412945 - Flags: review?(gal)
Attachment #412945 - Flags: review?(gal) → review+
Blocks: 529404
http://hg.mozilla.org/tracemonkey/rev/ac278013c799

I filed bug 529404 for switching to ES5 behavior -- we'll get it in after 3.6 is out, or after we're much closer than we are now, guessing a month or so from now.
Summary: Assignments to a property that has a getter but not a setter should only throw a TypeError in strict mode → Back out bug 478047 and revert to past behavior when setting a property that only has a getter
Whiteboard: fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/2fe1a3d6e672
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
wrong cset
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
http://hg.mozilla.org/mozilla-central/rev/ac278013c799
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
waldo, ecma_5/extensions/jstests.list needs to be listed in ecma_5/jstests.list
ditto for ecma_5/Types/jstests.list and ecma_5/jstests.list
You need to log in before you can comment on or make changes to this bug.