Last Comment Bug 702491 - JSPROP_READONLY shouldn't be set when parsing a jsobj property descriptor with a setter
: JSPROP_READONLY shouldn't be set when parsing a jsobj property descriptor wit...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla11
Assigned To: Bobby Holley (:bholley) (busy with Stylo)
:
Mentors:
Depends on: 711288 714663
Blocks: 645726 702353
  Show dependency treegraph
 
Reported: 2011-11-14 17:15 PST by Bobby Holley (:bholley) (busy with Stylo)
Modified: 2012-01-02 12:06 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch v1 (966 bytes, patch)
2011-11-14 17:24 PST, Bobby Holley (:bholley) (busy with Stylo)
no flags Details | Diff | Splinter Review
(Waldo way, incomplete) part 1 - Add the writable() accessor to PropertyDescriptor. v1 (2.96 KB, patch)
2011-12-02 13:30 PST, Bobby Holley (:bholley) (busy with Stylo)
no flags Details | Diff | Splinter Review
(Waldo way, incomplete) part 2 - Use |writable()| accessor instead of |& JSPROP_READONLY| whenever possible. v1 (3.62 KB, patch)
2011-12-02 13:30 PST, Bobby Holley (:bholley) (busy with Stylo)
no flags Details | Diff | Splinter Review
(Waldo way, incomplete) part 3 - Assert that we're only checking writable() for data descriptors. v1 (2.49 KB, patch)
2011-12-02 13:31 PST, Bobby Holley (:bholley) (busy with Stylo)
no flags Details | Diff | Splinter Review
Don't set JSPROP_READONLY for accessor properties. v1 (6.81 KB, patch)
2011-12-02 17:31 PST, Bobby Holley (:bholley) (busy with Stylo)
jwalden+bmo: review+
Details | Diff | Splinter Review
Don't set JSPROP_READONLY for accessor properties. v2 (7.67 KB, patch)
2011-12-13 11:09 PST, Bobby Holley (:bholley) (busy with Stylo)
jwalden+bmo: review+
Details | Diff | Splinter Review

Description Bobby Holley (:bholley) (busy with Stylo) 2011-11-14 17:15:41 PST
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.
Comment 1 Jeff Walden [:Waldo] (remove +bmo to email) 2011-11-14 17:21:47 PST
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.
Comment 2 Jeff Walden [:Waldo] (remove +bmo to email) 2011-11-14 17:22:56 PST
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".
Comment 3 Bobby Holley (:bholley) (busy with Stylo) 2011-11-14 17:24:14 PST
Created attachment 574485 [details] [diff] [review]
patch v1

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
Comment 4 Bobby Holley (:bholley) (busy with Stylo) 2011-11-14 17:28:50 PST
Oh, whoops - missed Waldo's most recent comment.

In that case, what's the way forward? Avoid checking JSPROP_READONLY for accessor props?
Comment 5 Jeff Walden [:Waldo] (remove +bmo to email) 2011-11-14 17:36:10 PST
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.
Comment 6 David Bruant 2011-11-15 04:15:02 PST
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.
Comment 7 David Bruant 2011-11-15 04:20:56 PST
... uh... I thought this bug was the same than bug 645726. But it clearly is not. My mistake. Forget the previous message. Sorry.
Comment 8 Jim Blandy :jimb 2011-11-15 10:51:56 PST
(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).
Comment 9 Bobby Holley (:bholley) (busy with Stylo) 2011-12-02 13:29:53 PST
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
Comment 10 Bobby Holley (:bholley) (busy with Stylo) 2011-12-02 13:30:33 PST
Created attachment 578701 [details] [diff] [review]
(Waldo way, incomplete) part 1 - Add the writable() accessor to PropertyDescriptor. v1
Comment 11 Bobby Holley (:bholley) (busy with Stylo) 2011-12-02 13:30:59 PST
Created attachment 578702 [details] [diff] [review]
(Waldo way, incomplete) part 2 - Use |writable()| accessor instead of |& JSPROP_READONLY| whenever possible. v1
Comment 12 Bobby Holley (:bholley) (busy with Stylo) 2011-12-02 13:31:22 PST
Created attachment 578703 [details] [diff] [review]
(Waldo way, incomplete) part 3 - Assert that we're only checking writable() for data descriptors. v1
Comment 13 Bobby Holley (:bholley) (busy with Stylo) 2011-12-02 17:31:52 PST
Created attachment 578787 [details] [diff] [review]
Don't set JSPROP_READONLY for accessor properties. v1

Attaching a patch to do things the Jorendorff way. Flagging Waldo for review.
Comment 14 Jeff Walden [:Waldo] (remove +bmo to email) 2011-12-07 19:46:53 PST
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.
Comment 15 Bobby Holley (:bholley) (busy with Stylo) 2011-12-13 11:09:02 PST
Created attachment 581335 [details] [diff] [review]
Don't set JSPROP_READONLY for accessor properties. v2

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. ;-)
Comment 16 Bobby Holley (:bholley) (busy with Stylo) 2011-12-13 11:10:00 PST
(also, the patch had to be rebased a bit on top of the objshrink changes, which explains some of the other differences).
Comment 17 Jeff Walden [:Waldo] (remove +bmo to email) 2011-12-13 13:02:11 PST
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.
Comment 18 Bobby Holley (:bholley) (busy with Stylo) 2011-12-13 14:21:41 PST
Cool, I've made the fixes.

This patch is ready to go when mozilla-inbound reopens.
Comment 19 Bobby Holley (:bholley) (busy with Stylo) 2011-12-15 11:43:46 PST
Pushed to mozilla-inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/49bd0c9665e5
Comment 20 Ed Morley [:emorley] 2011-12-16 06:09:10 PST
https://hg.mozilla.org/mozilla-central/rev/49bd0c9665e5

Note You need to log in before you can comment on or make changes to this bug.