Closed
Bug 523846
Opened 15 years ago
Closed 15 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)
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)
13.08 KB,
patch
|
brendan
:
review+
|
Details | Diff | Splinter Review |
6.20 KB,
patch
|
gal
:
review+
|
Details | Diff | Splinter Review |
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.)
Reporter | ||
Comment 1•15 years ago
|
||
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+
Reporter | ||
Comment 3•15 years ago
|
||
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 | ||
Updated•15 years ago
|
Assignee: general → jwalden+bmo
Status: NEW → ASSIGNED
Comment 5•15 years ago
|
||
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.
Assignee | ||
Comment 6•15 years ago
|
||
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)
Assignee | ||
Comment 7•15 years ago
|
||
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 8•15 years ago
|
||
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+
Assignee | ||
Comment 9•15 years ago
|
||
(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?
Reporter | ||
Comment 10•15 years ago
|
||
I agree.
Assignee | ||
Comment 11•15 years ago
|
||
Attachment #412945 -
Flags: review?(gal)
Updated•15 years ago
|
Attachment #412945 -
Flags: review?(gal) → review+
Assignee | ||
Comment 12•15 years ago
|
||
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
Comment 13•15 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/2fe1a3d6e672
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Comment 15•15 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/ac278013c799
Status: REOPENED → RESOLVED
Closed: 15 years ago → 15 years ago
Resolution: --- → FIXED
Comment 16•14 years ago
|
||
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/82f64078a448
status1.9.2:
--- → final-fixed
Comment 17•14 years ago
|
||
waldo, ecma_5/extensions/jstests.list needs to be listed in ecma_5/jstests.list
Comment 18•14 years ago
|
||
ditto for ecma_5/Types/jstests.list and ecma_5/jstests.list
Comment 19•14 years ago
|
||
fixed manifest http://hg.mozilla.org/tracemonkey/rev/3970759ca2c1
You need to log in
before you can comment on or make changes to this bug.
Description
•