They should behave like data properties. (function (y) { arguments.y = 2; var x = Object.create(arguments); x[0] = 3; assertEq(x[0], 3); assertEq(y, 2); })(1);

I put the test in ecma_5, since it uses Object.create. But you could create this object in ES3 too, and the behavior should be the same.

Comment on attachment 432591 [details] [diff] [review] v1 Withdrawing this. We want properties of the arguments object to remain slotless. The problem is that slotlessness and accessor-property-ness are represented by the same bit, JSPROP_SHARED. Here we want an ES5 "data property" without a slot.

(In reply to comment #2) > (From update of attachment 432591 [details] [diff] [review]) > Withdrawing this. We want properties of the arguments object to remain > slotless. > > The problem is that slotlessness and accessor-property-ness are represented by > the same bit, JSPROP_SHARED. > > Here we want an ES5 "data property" without a slot. Which would be indistinguishable from ES5 'accessor properties' under the hood, which appear to be 'data properties' to the ES5 and de-facto meta-object query API. So JSPROP_NOSLOT (use 0x80, shared with API-only input flag JSPROP_INDEX and function-flag-not-sprop-attr JSFUN_LAMBDA)? /be

Morphing this. The goal should be to remove the possibility of having both a slot and getter/setter (whether JSPropertyOp, or JSObject* representation flagged by JSPROP_[GS]ETTER). Then we can union slot with rawGetter or rawSetter (bug 558451 will make JSEmptyScope encapsulate a clasp unioned with the other raw*etter, but in either order there's room to overlay {clasp,slot} with {getter,setter}). Prototype properties which cannot be shadowed become ES5-conformant in the case of accessor properties. Data properties cannot be unshadowable -- must always be shadowable -- and by definition have a slot (do not have JSPROP_NOSLOT). Built-in JSProperyOp getter/setter pairs act like data properties without slots (we even use a fixed reserved slot under the hood), but therefore should be shadowable. No more JSPROP_SHARED, we can rename and use the same bit. API change, but it's the right thing, and it is time. Jason, can you sanity-check my words here? And, are you able to work on this or should someone cc'ed feel free to take it? /be

Making a property JSPROP_SHARED has three effects: 1. It makes it slotless. 2. It changes the behavior of assigning to the property via an object that inherits it from a prototype. In pseudocode: A = {}; JS_DefineProperty(cx, A, "prop", ..., JSPROP_SHARED); B = Object.create(A); B.prop = 0; // N.B. calls the setter instead of shadowing 3. In combination with PERMANENT, the property mostly passes for an own property. Comment 4 proposes making behavior #1 depend on whether the property has the default getter/setter or not. I am very much in favor. Comment 4 further proposes making behavior #2 depend on JSPROP_GETTER and JSPROP_SETTER rather than JSPROP_SHARED. The problem there is that xpc_qsDefineQuickStubs defines properties with JSPropertyOp accessors and JSPROP_SHARED. For those properties behavior #2 is crucial, and we don't want JSPROP_GETTER/SETTER. Of course we can't dispose of JSPROP_SHARED until we do something about behavior #3 as well. We'll get there.

Indeed, the exact test for (2) is (1): /* Don't clone a prototype property that doesn't have a slot. */ if (!sprop->hasSlot()) { in js_SetPropertyHelper, to decide not to shadow. (3) requires care currently to avoid deoptimizing due to js_GetMutableScope, but with scope elimination it should be no problem. My hope for JSPROP_GETTER | JSPROP_SETTER sufficing in lieu of JSPROP_NOSLOT or JSPROP_ACCESSOR forgot all about native non-stub JSPropertyOp getters and setters, but ok: we need a bit. We have a bit. We can rename it (+1 on ACCESSOR instead of NOSLOT). /be

(In reply to comment #6) > JSPROP_ACCESSOR forgot all about native non-stub JSPropertyOp getters and > setters, but ok: we need a bit. We have a bit. We can rename it (+1 on ACCESSOR > instead of NOSLOT). Or as Jason just suggested on IRC, give up on unioning slot with raw{G,S}etter and test for SPROP_INVALID_SLOT instead of JSPROP_ACCESSOR. Would be nice to free an attr bit, and (pending bug 558451 patch in full) alignment rounding may not free any space in JSScopeProperty if we did union slot with raw{G,S}etter anyway. /be

Morphing once again to focus on behavior #1 (of the three items listed in comment 5). === New bug description === A property should have a slot iff it has a non-default getter. After that: the properties of the arguments object should not be JSPROP_SHARED (this is the change in patch v1). These two changes together will fix the test case in comment 0 without making those properties slotful.

> A property should have a slot iff it has a non-default getter. Oops. That's negated. "iff it has the default getter" is correct. Rationale for depending on getter, not setter: I think if you use the default getter with a non-default setter, you want a slot. You might be using the setter to validate the assigned value or coerce it to the desired type, or to issue notifications or invalidate caches when it changes; but in any case you want the slot. Otherwise getting the property would always get JSVAL_VOID, which is awfully weird. Also note that in this case: var y = Object.create({}, {p: {set: print}}); we do NOT want a slot, even though we implement this as rawGetter == NULL.

(In reply to comment #9) > Rationale for depending on getter, not setter: I think if you use the default > getter with a non-default setter, you want a slot. You might be using the > setter to validate the assigned value or coerce it to the desired type, or to > issue notifications or invalidate caches when it changes; but in any case you > want the slot. Otherwise getting the property would always get JSVAL_VOID, > which is awfully weird. Yeah -- plus, watchpoints wrap using a cloned property with a JSPropertyOp setter and whatever slot and getter are in the watched prop. /be

Who will own this nicely focused bug? /be

Taking.

From bug 575997 comment 1: > I don't think RegExp and E4X need the shared-permanent hack, though: the > properties in question just need to be slotless and non-configurable. When bug > 552432 is fixed, these properties should no longer be JSPROP_SHARED. We should fix that in this bug, and add tests requiring the affected properties to act like data properties. For example: var x = Object.create(RegExp); x.$1 = 0; assertEq(x.hasOwnProperty("$1"), true); // fails assertEq(x.$1, 0); // fails

(In reply to comment #13) > For example: > > var x = Object.create(RegExp); > x.$1 = 0; > assertEq(x.hasOwnProperty("$1"), true); // fails > assertEq(x.$1, 0); // fails Not the example we want: RegExp.$1 is readonly (de-facto mess here). Comment 1 has a good test. /be

Should be safe. "What can go wrong?" - Homer S. /be

This fixes an ancient bug: js> o = {} ({}) js> p = {q:1} ({q:1}) js> o.__proto__ = p ({q:1}) js> o.__proto__ = null null js> o.__proto__ null js> o.__proto__ = p ({q:1}) js> o ({__proto__:{q:1}}) That's correct as far as __proto__ being magic goes (Rhino agrees). Without this patch, here's what happens: js> o = {} ({}) js> p = {q:1} ({q:1}) js> o.__proto__ = p ({q:1}) js> o.__proto__ = null null js> o.__proto__ = p ({q:1}) js> o null js> o.q js> o.__proto__ ({q:1}) The recreated o.__proto__ is a plain old data property. Jason, can you poke holes in this? Is it really this simple? It makes properties without slots but with JSPropertyOp-typed setters act a lot more like data properties, but shadowing a property with a shortid inherits that shortid, the HAS_SHORTID flag, and the prototype property's getter and setter. /be

Comment 1's testcase is wrong: (function (y) { arguments.y = 2; var x = Object.create(arguments); x[0] = 3; assertEq(x[0], 3); assertEq(y, 2); // 2 should be 1 here })(1); There are no named properties of arguments objects reflecting same-named formals, only indexed props. /be

The comment 16 with-patch results were slightly off, in that o.__proto__ became enumerable once assigned after being nulled. With this new patch: js> o = {} ({}) js> p = {q:1} ({q:1}) js> o.__proto__ = p ({q:1}) js> o.q 1 js> delete o.__proto__ false js> o.__proto__ ({q:1}) js> o.q 1 js> o.__proto__ = null null js> o.q js> o.__proto__ = p ({q:1}) js> o.q 1 js> o ({}) Note that Shape::shadowable (better name than canShadow, wrong verb given that |this| is the prototype property's shape) does not test JSPROP_SHARED at all if !shape->hasSlot(). We want to get rid of JSPROP_SHARED, and it will take several steps, so we don't want to add new tests of this attribute. (Getting rid of shortid is a different bargain, not materially altered here.) Here, the only issue is whether a property-setting operation is trying to shadow a proto-property that has no slot and a JSPropertyOp-type setter. If so, then the property will be shadowed -- and what's more, if the proto-property has a shortid then the related attrs, shortid and HAS_SHORTID flag, getter, and setter will be copied down. If no shortid, then the set creates an enumerable data property. /be

This passes the suite as far as I can tell. /be

This patch changes the behavior of assignment to an inherited property that has JSPROP_SHARED but not JSPROP_SETTER, is that right? Quick stub properties are like that.

(In reply to comment #21) > This patch changes the behavior of assignment to an inherited property that has > JSPROP_SHARED but not JSPROP_SETTER, is that right? Quick stub properties are > like that. It does not change the behavior if the proto-property has a shortid -- the shadow inherits attrs, shortid, getter and setter. Do quickstubs have shortids? /be

(In reply to comment #22) > (In reply to comment #21) > > This patch changes the behavior of assignment to an inherited property that has > > JSPROP_SHARED but not JSPROP_SETTER, is that right? Quick stub properties are > > like that. > > It does not change the behavior if the proto-property has a shortid -- the > shadow inherits attrs, shortid, getter and setter. Do quickstubs have shortids? No, they don't.

However, note that even if they did, this would cause such properties to be cloned onto each instance on which they were ever set... which would be a weird change in behavior.

(In reply to comment #24) > However, note that even if they did, this would cause such properties to be > cloned onto each instance on which they were ever set... which would be a weird > change in behavior. How is it weird? It could be too expensive, but that is not clear without data. /be

Weird is subjective, but here's why I feel this way. These properties have acted like accessor properties w.r.t. assignment for a long time. I think the ultimate target is to make these fully accessor properties--make Object.getOwnPropertyDescriptor return the getter and setter, the whole nine yards. So it seems weird to add an aspect of data property behavior now. You can observe the difference directly with .hasOwnProperty or Object.getOwnPropertyDescriptor, and you would notice it if you modify the original property on the prototype (objects with clones of the property would not see the change) or if you use a watchpoint.

(In reply to comment #27) > Weird is subjective, but here's why I feel this way. These properties have > acted like accessor properties w.r.t. assignment for a long time. I think the > ultimate target is to make these fully accessor properties--make > Object.getOwnPropertyDescriptor return the getter and setter, the whole nine > yards. So it seems weird to add an aspect of data property behavior now. I hear you, but doesn't this argue for Waldo's JSPROP_ACCESSOR patch, which would be more future-proof? What else can we do? My understanding of WebIDL and ES5 is that you'd have to use Object.defineProperty to override these proto-based quickstubbed DOM accessors. Can you guys please check me here, since I may be missing something. > You can observe the difference directly with .hasOwnProperty or > Object.getOwnPropertyDescriptor, and you would notice it if you modify the > original property on the prototype (objects with clones of the property would > not see the change) or if you use a watchpoint. Yes, for quickstubs it would be observable and it's not future-proof. But short of Waldo's JSPROP_ACCESSOR, I'm not sure what else to do. For properties such as arguments[0] accessed via a delegating object (as in the testcase based on comment 0), do we have such a future-hostility problem? ES5 models indexed properties of arguments using internal getters and setters but these are not observable. And aren't they shadowable by assignment? If not, this bug needs a new testcase! /be

This approach is much better. I was just thinking: the narrowest way to fix the particular test case at hand would be to make shadowable() return true if the property's setter is exactly ArgSetter.

(In reply to comment #30) > This approach is much better. > > I was just thinking: the narrowest way to fix the particular test case at hand > would be to make shadowable() return true if the property's setter is exactly > ArgSetter. I'd rather burn a bit (can take Waldo's suggestion of using 0x08, which is free, but using JSPROP_INDEX only on the engine's inside seems nicer if we need a real API-facing JSPROP_ new attribute). I suspect we have other cases like arguments elements. I'm just too tired to think of them atm. /be

Writable shared permanent properties having a JSPropertyOp setter that possibly should be per-instance ("own") data properties, excluding properties of objects that can't be referenced (Call, e.g.): Array length arguments elements (fixed by this bug's patch) strict arguments callee and caller RegExp lastIndex RegExp.input, RegExp.multiline statics Anyone reading, please double-check me if you can. Most of these can be fixed here using SHADOWABLE. /be

I can't finish this tonight. Fading. The main thing I still want to look at is whether PolyIC also needs changes.

Comment on attachment 497707 [details] [diff] [review] more narrowly targeted fix >+ return hasSlot() || (!hasSetterValue() && (attrs & JSPROP_SHADOWABLE)); This could be return hasSlot() || (attrs & JSPROP_SHADOWABLE); Right? r=me either way. jstracer doesn't need an update for this since the change is before SetPropHit. However my SetPropHit-ectomy will need a slight adjustment, since in that world the tracer has to distinguish adding assignments from non-adding before the instruction executes. PolyIC actually disables in these cases (assigning to any shared property with a setter), which seems like it might be slow. Has anyone looked at that?

http://hg.mozilla.org/tracemonkey/rev/df69f4793470 > However my SetPropHit-ectomy will need a slight adjustment, since in that world > the tracer has to distinguish adding assignments from non-adding before the > instruction executes. I just landed, if you rebase the SetPropHit-ectomy I will review it by tomorrow noon. Thanks. > PolyIC actually disables in these cases (assigning to any shared property with > a setter), which seems like it might be slow. Has anyone looked at that? Separate bug, cc'ing bhackett and dvander who may know of one on file, or file, or something (comment about WONTFIX nature of JITting these cases, pending more real-world data?). /be

(In reply to comment #34) > Comment on attachment 497707 [details] [diff] [review] > more narrowly targeted fix > > >+ return hasSlot() || (!hasSetterValue() && (attrs & JSPROP_SHADOWABLE)); > > This could be > return hasSlot() || (attrs & JSPROP_SHADOWABLE); > > Right? Right -- I was playing it too safe. JSPROP_SHADOWABLE is internal-only but it could be useful with function-based accessors as well as with PropertyOp-based properties (as in the patch). /be

(In reply to comment #34) > jstracer doesn't need an update for this since the change is before SetPropHit. > However my SetPropHit-ectomy will need a slight adjustment, since in that world > the tracer has to distinguish adding assignments from non-adding before the > instruction executes. Jason, following up here: did your SetPropHit-ectomy make the slight adjustment? /be

Bugzilla broken? I sweat I didn't touch either of Component or TM. /be