JSPROP_READONLY applies to all property descriptors, both value-based and accessor-based: http://mxr.mozilla.org/mozilla-central/source/js/src/jsapi.h#1672 However, the 'writable' attribute in a JSObject property descriptor applies only to value property descriptors. In particular, when converting a property descriptor for a native, writable, accessor-based property to a JSObject, we don't include 'writable': http://mxr.mozilla.org/mozilla-central/source/js/src/jsobj.cpp#1774 Unfortunately, when we convert JSObject property descriptors back to native ones, we _default_ to JSPROP_READONLY, and only clear the bits if 'writable' is set: http://mxr.mozilla.org/mozilla-central/source/js/src/jsobj.cpp#1990 The net effect here is that the obvious implementation of a forwarding proxy doesn't work. I implement getOwnPropertyDescriptor(name) as a call to Object.getOwnPropertyDescriptor(wrappedObject, name), which causes the property descriptor to transform from PropDesc to JSObject and back again. In the process we end up with a JSPROP_READONLY where there previously was none, which makes the setter stop working for no apparent reason. I think I've got a patch that should fix this. I'm running it through the JS tests to be sure, and then I'll post it for review.

We should not be querying writable() or (attrs & JSPROP_READONLY) if the property is an accessor property. Not setting the attribute may fix some users, but really, the users shouldn't be asking the question in the first place. (Note the commented-out assertion in the writable() method that the descriptor in question is a data descriptor.) At one point I don't think we were super-far off from never asking that question of accessor descriptors. I'm not sure where we sit with that now. I've wanted to fix this for awhile but never found the time to do it.

Further to the point, note that descriptors in the spec have [[Enumerable]] and [[Configurable]], and either 1) [[Value]] and [[Writable]], or 2) [[Getter]] and [[Setter]]. Accessors simply do not have any notion of writability which we should be exposing or permitting to be "set".

This seems to do the right thing in my local testing, and makes sense IMO. Flagging Waldo for review. I did make jstestbrowser, and got a few errors related to DST. I don't think they're related, but this is the first time I've run js tests. Including the failures below. REFTEST TEST-UNEXPECTED-FAIL | file:///files/mozilla/repos/d/js/src/tests/jsreftest.html?test=ecma/Date/15.9.5.35-1.js | TDATE = new Date(0);(TDATE).setUTCMonth(5,4);TDATE.getHours() wrong value item 53 REFTEST TEST-UNEXPECTED-FAIL | file:///files/mozilla/repos/d/js/src/tests/jsreftest.html?test=js1_5/extensions/toLocaleFormat-01.js | Date.toLocaleFormat(%I) Expected value '12', Actual value '0' item 7 REFTEST TEST-UNEXPECTED-FAIL | file:///files/mozilla/repos/d/js/src/tests/jsreftest.html?test=js1_5/Regress/regress-58116.js | Compute Daylight savings time correctly regardless of year Section 1 of test - 1970-07-1 Expected value '-120', Actual value '-60' item 1 REFTEST TEST-UNEXPECTED-FAIL | file:///files/mozilla/repos/d/js/src/tests/jsreftest.html?test=js1_5/Regress/regress-58116.js | Compute Daylight savings time correctly regardless of year Section 2 of test - 1965-07-1 Expected value '-120', Actual value '-60' item 2 REFTEST TEST-UNEXPECTED-FAIL | file:///files/mozilla/repos/d/js/src/tests/jsreftest.html?test=js1_5/Regress/regress-58116.js | Compute Daylight savings time correctly regardless of year Section 3 of test - 0000-07-1 Expected value '-120', Actual value '-60' item 3 Review: :jwalden+bmo

Oh, whoops - missed Waldo's most recent comment. In that case, what's the way forward? Avoid checking JSPROP_READONLY for accessor props?

Yeah. Users should deciding how to handle a descriptor based on whether it's a data descriptor or an accessor descriptor, then only asking after writability in the data case. I can't think of a reason why asking for writability for an accessor makes sense.

I realize that some work has already been done to fix this bug, but I'd like to inform you of the new proxy proposal: wiki.ecmascript.org/doku.php?id=strawman:direct_proxies I won't dive into the details now, but it is very likely that if this proposal is accepted at the next TC39 meeting (end of November), proxies internals are going to need a major rewrite (which is very likely to be a simplification). For that reason, unless the current patch actually fixes the bug, I'd like to encourage you to stop working on the current proxy code base as this code base may be thrown away in a month.

... uh... I thought this bug was the same than bug 645726. But it clearly is not. My mistake. Forget the previous message. Sorry.

(In reply to Jeff Walden (remove +bmo to email) from comment #1) > We should not be querying writable() or (attrs & JSPROP_READONLY) if the > property is an accessor property. Not setting the attribute may fix some > users, but really, the users shouldn't be asking the question in the first > place. (Note the commented-out assertion in the writable() method that the > descriptor in question is a data descriptor.) > > At one point I don't think we were super-far off from never asking that > question of accessor descriptors. I'm not sure where we sit with that now. > I've wanted to fix this for awhile but never found the time to do it. bholley, what Waldo says here sounds exactly right to me, too. The JSPROP_READONLY bit of PropDesc::attrs should be treated as dead when JSPROP_GETTER or JSPROP_SETTER are set. One way to work through it would be to make 'attrs' a private field of PropDesc, make sure all users are going through PropDesc::writable, and then have that say JS_ASSERT(!isAccessorDescriptor).

So doing things the way Waldo wanted has turned out to be quite tricky. I've got patches to make us access JSPROP_READONLY via writable (for PropertyDescriptor and PropDesc, in addition to Shape), and to assert within writable() that we're a data descriptor. But things got tricky when it came to fixing the asserts, largely because of confusing surrounding native getters/setters. So I'm going to post the patches I have for reference (in case Waldo wants to pick up where I left off), and then do this the easier way. The IRC log is here for reference: bholley: dvander: and here: http://mxr.mozilla.org/mozilla-central/source/js/src/frontend/BytecodeEmitter.cpp#1783 ? check isDataDescriptor() before !writable() and kill the call to hasDefaultGetter()? dvander: bholley, yup bholley: dvander: and this is all ok, despite the fact that we can have a shape with isDataDescriptor() but !hasDefault{Getter,Setter}(), via JSPropertyOps? bholley: (since isDataDescriptor only checks JSPROP_GETTER and JSPROP_SETTER) • bholley notes that native property ops are kind of broken anyway dvander: bholley, i'm not exactly sure what it means to have one of the internal getters, but not setters dvander: bholley, most likely the hasSlot thing will fail bholley: dvander: well, even if we have both a native getter and a native setter, we still have !isDataDescriptor() dvander: how could you have an isDataDescriptor but have non-default getters/setters? bholley: dvander: er, the other way around from what I said. We can have isDataDescriptor() with non-default getters/setters (like you said), because if we have native getters and setters we don't have JSPROP_GETTER and JSPROP_SETTER enabled bholley: dvander: since AFAIK JSPROP_SETTER and JSPROP_GETTER refer only to object accessors dvander: bholley, what exactly does that mean though? bholley: dvander: we have a JSObject with native accessors via JSPropertyOps dvander: bholley, i mean, semantically, is that *actually* still a data descriptor? or is it some weird thing bholley: dvander: I don't think it's a data descriptor, no, since there are accessors • bholley is not the expert here, though dvander: bholley, so then is it safe to say that isDataDescriptor() is !hasDefaultGetter() || !hasDefaultSetter() ? dvander: if not dvander: the test has to become dvander: isDataDescriptor() && hasDefaultSetter() && writable() bholley: dvander: hm, yeah. I wonder if isDataDescriptor() could just be redefined to !hasDefaultGetter() && !hasDefaultSetter() bholley: maybe jorendorff could weigh in? jorendorff: No, that is not possible, because of magical data descriptors like the one for Array length properties bholley: jorendorff: so if JM wants to know if something is a data descriptor, what should it check? !hasDefaultSetter() && !hasDefaultGetter()? bholley: maybe we could make a isRealDataDescriptor()... dvander: data descriptor is an ES5 term, right? and the internal setters are something special dvander: so yeah, if you want to make one helper we'll need our own terminology jorendorff: What it's doing right now is fine. It still has to check either hasDefaultSetter() or hasDefaultGetter() depending on what it's doing. dvander: jorendorff, i think the issue is that waldo wants writable() to assert that it's being called on a data descriptor jorendorff: then he can pony up the patch, imho jorendorff: bholley should do what we talked about earlier -- just don't set READONLY erroneously jorendorff: and assert that we don't dvander: fair enough! bholley: jorendorff, Waldo, sounds good! jorendorff: i'm not totally sure, but my impression was Waldo won't object too much, it's more of a "would've been nice" kind of situation bholley: jorendorff: where exactly should I assert that we don't set them erroneously? jorendorff: bholley: js::Shape::attrs is private -- just assert in each place where we touch that field jorendorff: i mean, where we modify it jorendorff: not where we read it bholley: jorendorff: ok, got it

Attaching a patch to do things the Jorendorff way. Flagging Waldo for review.

Comment on attachment 578787 [details] [diff] [review] Don't set JSPROP_READONLY for accessor properties. v1 Review of attachment 578787 [details] [diff] [review]: ----------------------------------------------------------------- Good enough for now.

So the last patch hit one of the assertions during the jit-tests. I fixed up the issue, and added another assertion for good measure. This one went green on try: https://tbpl.mozilla.org/?tree=Try&rev=8a9f51c167fa Flagging Waldo for re-review, just to be safe. The interdiff between this and the last patch is quite small. ;-)

(also, the patch had to be rebased a bit on top of the objshrink changes, which explains some of the other differences).

Comment on attachment 581335 [details] [diff] [review] Don't set JSPROP_READONLY for accessor properties. v2 Review of attachment 581335 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsobj.cpp @@ +1744,5 @@ > PropDesc::initFromPropertyDescriptor(const PropertyDescriptor &desc) > { > pd.setUndefined(); > attrs = uint8(desc.attrs); > + JS_ASSERT_IF((attrs & JSPROP_READONLY), !(attrs & (JSPROP_GETTER | JSPROP_SETTER))); & is over-parenthesized in the first condition. ::: js/src/jsscopeinlines.h @@ +163,5 @@ > { > JS_ASSERT(base); > JS_ASSERT(!JSID_IS_VOID(propid)); > JS_ASSERT_IF(isMethod(), !base->rawGetter); > + JS_ASSERT_IF((attrs & JSPROP_READONLY), !(attrs & (JSPROP_GETTER | JSPROP_SETTER))); & over-parenthesized again.

Cool, I've made the fixes. This patch is ready to go when mozilla-inbound reopens.

Pushed to mozilla-inbound: http://hg.mozilla.org/integration/mozilla-inbound/rev/49bd0c9665e5