Object.preventExtensions and Object.isExtensible are specified by 15.2.3.10 and 15.2.3.13 in the ES5 draft: http://wiki.ecmascript.org/doku.php?id=es3.1:es3.1_proposal_working_draft

Per ES5, property-adds should silently fail; property-changes (including to/from function values, sigh) should work. This patch does neither.

Waldo, whats the status of this? Any progress?

Status is that this is blocked on Object.getOwnPropertyNames, bug 518663. It's simple enough to make adding new properties fail -- the problem is you have to make sure the failing starts after all existing properties are already there -- including properties that exist only through resolve hooks.

http://wiki.whatwg.org/wiki/Web_ECMAScript#Object_Properties points out that when this is implemented, it should also kill __proto__-changes.

Yes. ES5 points this out too ;).

This bug depends on Brendan's scope-removal work in bug 558451.

(In reply to comment #8) > This bug depends on Brendan's scope-removal work in bug 558451. Jorendorff's reviewing, but the dependency is weaker than it might seem at first glance. The bug 558451 patch queue builds on the JSObject changes for fatvals, which left a flags word in JSObject (now 8 bits, all used). I moved SEALED from JSScope::flags to JSObject::flags in the 558451 mq. Is it retaskable as NON_EXTENSIBLE, and the rest can be done with property attribute fiddling? I hope so. If so, you are good to go in parallel. /be

That seems true, but for it to properly integrate with tracing I think we would need to add extensibility guards to enough places that I am skeptical it wouldn't show up in benchmarks. For that reason, at least, I think I'm still lined up behind you. I spent some time today resurrecting previous patches for here and for bug 492844 and bug 492845, then converting them to use a flags-based mechanism rather than one kept in the object scope. It shouldn't be much difficulty rebasing that work atop yours when it's ready.

(In reply to comment #10) > That seems true, but for it to properly integrate with tracing I think we would > need to add extensibility guards to enough places that I am skeptical it > wouldn't show up in benchmarks. No benchmark uses ES5. Anything that sets the SEALED (you'll rename it NON_EXTENSIBLE ;-) flag also generates and sets an "own shape" for the object. Shape guards we already emit when tracing will catch this. > For that reason, at least, I think I'm still lined up behind you. Cool, I have other reasons to want that ;-). Mainly rebase fatigue and chasing down tryserver issues taking their toll. /be

Okay, I think this is good to go now. I'm not going to be around until Monday, so feel free to punt the review time-wise (or to a different person) as necessary.

Comment on attachment 473777 [details] [diff] [review] Rebased >+JS_PUBLIC_API(JSBool) >+JS_SealObject(JSContext *cx, JSObject *obj, JSBool deep) I would rather see this API removed, and add JS_DeepFreezeObject or JS_FreezeObjectDeeply, than drag out the "seal" meaning confusion. Post to m.d.t.js-engine and just do it. Followup bug ok. >- /* Walk slots in obj and if any value is a non-null object, seal it. */ >- for (uint32 i = 0, n = obj->freeslot; i != n; ++i) { >- const Value &v = obj->getSlot(i); >- if (i == JSSLOT_PRIVATE && (obj->getClass()->flags & JSCLASS_HAS_PRIVATE)) >- continue; >- if (v.isPrimitive()) >- continue; >- if (!JS_SealObject(cx, &v.toObject(), deep)) >- return false; >- } >+ /* Walk slots in obj and if any value is an object, seal it. */ >+ // XXX WRITE ME! Here you removed the code that is still needed so long as the API including its deep flag parameter exists. If you're going to keep JS_SealObject for now, restore the loop -- it ought to work as well as it did before, so long as you don't diverge by recursing on already-frozen objects. >+/* >+ * Equivalent to Object.freeze (called recursively, if needed). Deprecated >+ * as this previously-existing name is a clear false cognate of ES5's >+ * concept of sealing via Object.seal. >+ */ >+extern JS_DEPRECATED JS_PUBLIC_API(JSBool) >+JS_SealObject(JSContext *cx, JSObject *obj, JSBool deep); Deprecation is not our way, and seldom effective. Remove if you can, now or in a followup, but don't mangle with XXX deep breakage! >+JSBool >+array_beforePreventExtensions(JSContext *cx, JSObject *obj, AutoIdVector &props) This optional Class hook's name is way too long, and the API isn't right. r?'ing gal but it should line up with http://wiki.ecmascript.org/doku.php?id=harmony:proxies better: 1. Call it "fix". 2. Have it return a PropDescArray. >+{ >+ JS_ASSERT(obj->isDenseArray()); >+ >+ return obj->makeDenseArraySlow(cx) && >+ GetPropertyNames(cx, obj, JSITER_HIDDEN | JSITER_OWNONLY, props); Why do dense arrays need to be slow-ified to have extensions prevented? The JSObject::NOT_EXTENSIBLE flag is universal for all objects. >+JSObject::modifyAllPropertyAttributes(JSContext *cx, JSObject::AttributeChangeType change) >+{ >+ assertSameCompartment(cx, this); >+ >+ AutoIdVector props(cx); >+ if (isExtensible()) { >+ if (!preventExtensions(cx, props)) >+ return false; >+ } else { >+ if (!GetPropertyNames(cx, this, JSITER_HIDDEN | JSITER_OWNONLY, props)) >+ return false; >+ } >+ >+ JS_ASSERT(!isDenseArray()); >+ >+ if (change == SEAL_ATTRIBUTES) { >+ for (size_t i = 0, len = props.length(); i < len; i++) { >+ jsid id = props[i]; >+ >+ uintN attrs = 0; >+ if (!getAttributes(cx, id, &attrs)) >+ return false; >+ >+ if (attrs & JSPROP_PERMANENT) >+ continue; >+ >+ attrs |= JSPROP_PERMANENT; >+ if (!setAttributes(cx, id, &attrs)) >+ return false; >+ } >+ } else { >+ JS_ASSERT(change == FREEZE_ATTRIBUTES); >+ >+ for (size_t i = 0, len = props.length(); i < len; i++) { >+ jsid id = props[i]; >+ >+ uintN attrs = 0; >+ if (!getAttributes(cx, id, &attrs)) >+ return false; >+ >+ if ((attrs & JSPROP_PERMANENT) && >+ (!(attrs & (JSPROP_GETTER | JSPROP_SETTER)) || (attrs & JSPROP_READONLY))) { >+ continue; >+ } >+ >+ /* NB: JSPROP_READONLY is ignored for accessor descriptors. */ >+ attrs |= JSPROP_PERMANENT | JSPROP_READONLY; >+ if (!setAttributes(cx, id, &attrs)) >+ return false; >+ } >+ } >+ >+ return true; >+} Cite ES5 clauses if you can. After the (short) common prologue, this method is really two methods. Split it up and lose the AttributeChangeType enum, or else wind it tigher: one loop with condition-checking in the middle. >@@ -5087,6 +5163,18 @@ js_SetPropertyHelper(JSContext *cx, JSOb > added = false; > if (!shape) { > /* >+ * Do this without locking obj, because sharing prior to changing an >+ * object's extensibility is inherently racy. >+ */ No need for this comment with JS_THREADSAFE going away soon. >@@ -5433,8 +5521,14 @@ CheckAccess(JSContext *cx, JSObject *obj > switch (mode & JSACC_TYPEMASK) { > case JSACC_PROTO: > pobj = obj; >- if (!writing) >+ if (writing) { >+ if (!obj->isExtensible()) { >+ JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_OBJECT_NOT_EXTENSIBLE); >+ return false; >+ } Why is this here? CheckAccess (former JSObjectOp) is a helper that dispatches the Class::checkAccess op, which allows classes to specialize. Any universal not-extensible error should be checked for above this layer, in obj_setProto. >@@ -6268,11 +6362,13 @@ js_DumpObject(JSObject *obj) > clasp = obj->getClass(); > fprintf(stderr, "class %p %s\n", (void *)clasp, clasp->name); > >+ if (!obj->isExtensible()) >+ fprintf(stderr, "not extensible\n"); >+ > fprintf(stderr, "flags:"); > uint32 flags = obj->flags; > if (flags & JSObject::DELEGATE) fprintf(stderr, " delegate"); > if (flags & JSObject::SYSTEM) fprintf(stderr, " system"); >- if (flags & JSObject::SEALED) fprintf(stderr, " sealed"); > if (flags & JSObject::BRANDED) fprintf(stderr, " branded"); > if (flags & JSObject::GENERIC) fprintf(stderr, " generic"); > if (flags & JSObject::METHOD_BARRIER) fprintf(stderr, " method_barrier"); Why not follow the flags & ... pattern and put not_extensible on the same line with other flags? >@@ -632,12 +632,10 @@ JSObject::addProperty(JSContext *cx, jsi > JS_ASSERT(!JSID_IS_VOID(id)); > > /* >- * You can't add properties to a sealed object. But note well that you can >- * change property attributes in a sealed object, even though that replaces >- * a Shape * in the scope's hash table -- but no id is added, so the object >- * remains sealed. >+ * You can't add properties to a non-extensible object, but you can change >+ * attributes of properties in such objects. > */ >- if (sealed()) { >+ if (!isExtensible()) { > reportReadOnlyScope(cx); > return NULL; > } >@@ -739,11 +737,6 @@ JSObject::putProperty(JSContext *cx, jsi > > JS_ASSERT(!JSID_IS_VOID(id)); > >- if (sealed()) { >- reportReadOnlyScope(cx); >- return NULL; >- } >- > NormalizeGetterAndSetter(cx, this, id, attrs, flags, getter, setter); > > /* Search for id in order to claim its entry if table has been allocated. */ >@@ -916,11 +909,6 @@ JSObject::changeProperty(JSContext *cx, > bool > JSObject::removeProperty(JSContext *cx, jsid id) > { >- if (sealed()) { >- reportReadOnlyScope(cx); >- return false; >- } Great to see these now-clearly-misplaced-per-ES5 checks go!

(In reply to comment #14) > >+JSBool > >+array_beforePreventExtensions(JSContext *cx, JSObject *obj, AutoIdVector &props) > > This optional Class hook's name is way too long, and the API isn't right. > r?'ing gal but it should line up with > http://wiki.ecmascript.org/doku.php?id=harmony:proxies better: > > 1. Call it "fix". > 2. Have it return a PropDescArray. 3. Some way to "return undefined" meaning "refuse to be fixed" per the proxies spec. Waldo, I can r+ a fast followup if you have one -- heard you're on vacation soon but I'm around now and will be online on and off tonight. /be

I don't know if I do. The things here are likely easier to address than strict-mode-this work which I have half/mostly-completed (working on now), and I feel two patches mostly done are likely better than one patch all done and one patch only half-done. Plus I have a review queue to exhaust. Feeling frazzled but pushing through now, keeping eyes on the prize...

I r+'ed the isSealed, etc. patch (bug 492845) so yeah, this bug and that seem to be poised to land soon. For the "return undefined" convention for Class::fix, that hook could return true (no exception thrown or error reported) but with a PropDescArray pointer passed by its address as an out param set to null. /be

I'm pushing this keep-working thing way too late tonight (to-morning?), going to have to cut it off now or I'll be repackaging food, assembling gear, and being busy right up until I have to be waking up. Reviews and finishing off strict-this (which is likely less tricksy and commentable than this) prevented me from dealing with all the comments here (I got down to the "fix" comment, but not the PropDescArray bit). I do like the "fix" name, by the way. You might be right about not needing to slowify, but I'm not so sure. Would every setelem on a dense array need to check for extensibility or non-holeness? I didn't think setelem did shape checks if it was a dense array. But maybe I'm behind the shape-times. And my brain is zonked out now, so maybe, alternately, I'm missing something. Re "this method is really two methods": yeah, looks nicely addressable with one method templatized on a PropertyBehavior class that has a method to check for continue-ness and a field containing attribute or-bits, come to think of it. I seem to remember the CheckAccess bit being the lowest place where prototypes could be mutated, but it's been a little while, and it's hazy. I dislike working with raw flag-bits: requires more knowledge of object structure and layout, versus just knowing "this method tells me what I want". Feel free to use flags if you want consistency for now. And that's all, I'm out. Whoever ends up pushing this the last few feet, I owe you one.

One last: I don't know whether the seal/freeze patch needs more changes (haven't read that bugmail, not going to now), but if more are needed, I'm not going to be the one to make them -- too late tonight.

(In reply to comment #18) > You might be right about not needing to slowify, but I'm not so sure. Would > every setelem on a dense array need to check for extensibility or non-holeness? You're right, there'd need to be checks filling holes and extending length. I'm thinking we can take the heat there, but your slowify approach is certainly simpler and less code. Let's go with it until someone complains that frozen dense arrays are slower than non-frozen dense arrays. /be

jimb will take this over the finish line.

> Let's go with it until someone complains that frozen > dense arrays are slower than non-frozen dense arrays. Slower in what ways? At what operations?

Hi, Brendan. I think I've made the changes you requested; if you could give this a quick review, it'll help us get ES5 in.

Comment on attachment 475931 [details] [diff] [review] Implement Object.preventExtensions, Object.isExtensible. >+/* >+ * Freeze obj, and all objects it refers to, recursively. This will not >+ * recurse through non-extensible objects, on the assumption that those are >+ * already deep-frozen. ..12345678901234567890123456789012345678901234567890123456789012345678901234567890Nit: seems rewrappable with wm=79. >+bool >+JSObject::seal(JSContext *cx) >+{ >+ assertSameCompartment(cx, this); >+ >+ AutoIdVector props(cx); >+ if (isExtensible()) { >+ if (!preventExtensions(cx, props)) >+ return false; >+ } else { >+ if (!GetPropertyNames(cx, this, JSITER_HIDDEN | JSITER_OWNONLY, props)) >+ return false; >+ } Waldo mentioned templatizing to common this code between freeze and seal, but I see a prior problem now: JSObject::preventExtensions already does GetPropertyNames or calls a custom fix ObjectOp, which must reify lazy properties. So if (!isExtensible()) (the else clause here), we must have already gone through JSObject::preventExtensions -- it is the funnel to the one place that sets the NOT_EXTENSIBLE flag. So I don't see why we want the else clause, and that makes the if-if (really just an if(&&)) small enough to duplicate here and in freeze. >+ >+ JS_ASSERT(!isDenseArray()); >+ >+ for (size_t i = 0, len = props.length(); i < len; i++) { >+ jsid id = props[i]; >+ >+ uintN attrs = 0; >+ if (!getAttributes(cx, id, &attrs)) >+ return false; >+ >+ if (attrs & JSPROP_PERMANENT) >+ continue; >+ >+ attrs |= JSPROP_PERMANENT; >+ if (!setAttributes(cx, id, &attrs)) >+ return false; >+ } The loop duplication isn't bad but it does seem common-worthy via templates or an inline helper. Your call. >+inline bool >+JSObject::preventExtensions(JSContext *cx, js::AutoIdVector &props) > { >+ JS_ASSERT(isExtensible()); >+ >+ if (js::FixOp fix = getOps()->fix) { >+ if (!fix(cx, this, props)) >+ return false; >+ } else { >+ if (!GetPropertyNames(cx, this, JSITER_HIDDEN | JSITER_OWNONLY, props)) >+ return false; >+ } >+ > if (isNative()) >+ extensibleShapeChange(cx); >+ >+ flags |= NOT_EXTENSIBLE; >+ return true; > } Here's JSObject::preventExtensions for reference. Jason raised the question of extensibleShapeChange with respect to attribute changes in freeze and seal. It's a good point: generateOwnShape done via this code called from the top of freeze or seal, followed by a loop calling js_SetAttributes, will regenerate the |this| object's shape unnecessarily. Better to do the preventExtensions call last, not first, in freeze and seal, if that doesn't break anything (it shouldn't). >+++ b/js/src/jsproxy.cpp >@@ -936,6 +936,7 @@ JS_FRIEND_API(Class) ObjectProxyClass = > NULL, /* enumerate */ > NULL, /* typeof */ > proxy_TraceObject, >+ NULL, /* beforePreventExtensions */ > NULL, /* thisObject */ > proxy_Finalize, /* clear */ > } >@@ -998,6 +999,7 @@ JS_FRIEND_API(Class) FunctionProxyClass > NULL, /* enumerate */ > proxy_TypeOf_fun, > proxy_TraceObject, >+ NULL, /* beforePreventExtensions */ Stale comments -- should say "fix". >+++ b/js/src/jsscope.cpp >@@ -721,12 +721,10 @@ JSObject::addProperty(JSContext *cx, jsi > JS_ASSERT(!JSID_IS_VOID(id)); > > /* >- * You can't add properties to a sealed object. But note well that you can >- * change property attributes in a sealed object, even though that replaces >- * a Shape * in the scope's hash table -- but no id is added, so the object >- * remains sealed. >+ * You can't add properties to a non-extensible object, but you can change >+ * attributes of properties in such objects. > */ >- if (sealed()) { >+ if (!isExtensible()) { > reportReadOnlyScope(cx); > return NULL; > } Jason caught this too -- this belongs in addPropertyCommon if you can delay checking till then without issue (should be able to). Otherwise the putProperty->addPropertyCommon case is not covered. >@@ -4881,6 +4881,18 @@ xml_trace(JSTracer *trc, JSObject *obj) > JS_CALL_TRACER(trc, xml, JSTRACE_XML, "private"); > } > >+namespace { >+ >+JSBool >+xml_beforePreventExtensions(JSContext *cx, JSObject *obj, AutoIdVector &props) This should be named xml_fix now, here and below. >diff --git a/js/src/methodjit/PolyIC.cpp b/js/src/methodjit/PolyIC.cpp >--- a/js/src/methodjit/PolyIC.cpp >+++ b/js/src/methodjit/PolyIC.cpp >@@ -559,8 +559,6 @@ class SetPropCompiler : public PICStubCo > return disable("dense array"); > if (!obj->isNative()) > return disable("non-native"); >- if (obj->sealed()) >- return disable("sealed"); > > Class *clasp = obj->getClass(); > >@@ -590,8 +588,6 @@ class SetPropCompiler : public PICStubCo > > if (!holder->isNative()) > return disable("non-native holder"); >- if (holder->sealed()) >- return disable("sealed holder"); > > if (!shape->writable()) > return disable("readonly"); Don't we need some kind of !holder->isExtensible() check in the methodjit? /be

(In reply to comment #22) > > Let's go with it until someone complains that frozen > > dense arrays are slower than non-frozen dense arrays. > > Slower in what ways? At what operations? Array element and length acceses. Slower is something you should measure but it will be noticeable on any array-intensive benchmark. Dense arrays are our "fast arrays". Slow arrays are dense arrays that became too sparse or had a named property (other than length of course) set on them. We want to refactor to make dense-array optimizations universal for all objects but there will always be a slow case for Array. Rather than add isExtensible() tests in our fast paths for hole-filling and extension-by-assignment, making the array slow seems fine for now. At the least, optimizing frozen arrays to remain dense if the only reason they were made slow with this bug's patch was to make them non-extensible shouldn't hold up this bug. Followup bug fodder. /be

(In reply to comment #25) > It's a good point: generateOwnShape done via this code called from the top of > freeze or seal, followed by a loop calling js_SetAttributes, will regenerate > the |this| object's shape unnecessarily. Better to do the preventExtensions > call last, not first, in freeze and seal, if that doesn't break anything (it > shouldn't). Won't the first js_SetAttributes put the object into dictionary mode anyway? If so, this won't save any shapes. It would be simple enough to test in the shell, using shapeOf().

You can optimize harder, and to freeze/seal a native object, - put the object into dictionary mode, giving it its own shapeid, - clear the extensible bit, - walk the linked list of Shapes and mutate the bits as desired. This only uses up one shapeid, and it's a small amount of code -- but I'm not sure this is what Brendan's proposing.

(In reply to comment #25) > >+bool > >+JSObject::seal(JSContext *cx) > >+{ > >+ assertSameCompartment(cx, this); > >+ > >+ AutoIdVector props(cx); > >+ if (isExtensible()) { > >+ if (!preventExtensions(cx, props)) > >+ return false; > >+ } else { > >+ if (!GetPropertyNames(cx, this, JSITER_HIDDEN | JSITER_OWNONLY, props)) > >+ return false; > >+ } > > Waldo mentioned templatizing to common this code between freeze and seal, but I > see a prior problem now: > > JSObject::preventExtensions already does GetPropertyNames or calls a custom fix > ObjectOp, which must reify lazy properties. So if (!isExtensible()) (the else > clause here), we must have already gone through JSObject::preventExtensions -- > it is the funnel to the one place that sets the NOT_EXTENSIBLE flag. > > So I don't see why we want the else clause, and that makes the if-if (really > just an if(&&)) small enough to duplicate here and in freeze. An object may be non-extensible, but not have had its properties marked non-configurable. In that case, even though !isExtensible, we still need to enumerate the properties for the loop below to traverse. > The loop duplication isn't bad but it does seem common-worthy via templates or > an inline helper. Your call. I've pulled everything out into a common function, and managed to share the loop bodies as well. > Here's JSObject::preventExtensions for reference. Jason raised the question of > extensibleShapeChange with respect to attribute changes in freeze and seal. > It's a good point: generateOwnShape done via this code called from the top of > freeze or seal, followed by a loop calling js_SetAttributes, will regenerate > the |this| object's shape unnecessarily. Better to do the preventExtensions > call last, not first, in freeze and seal, if that doesn't break anything (it > shouldn't). I didn't see how this will help; Jason will be commenting himself, concurring. > Stale comments -- should say "fix". Done. > >+++ b/js/src/jsscope.cpp > >@@ -721,12 +721,10 @@ JSObject::addProperty(JSContext *cx, jsi > > JS_ASSERT(!JSID_IS_VOID(id)); > > > > /* > >- * You can't add properties to a sealed object. But note well that you can > >- * change property attributes in a sealed object, even though that replaces > >- * a Shape * in the scope's hash table -- but no id is added, so the object > >- * remains sealed. > >+ * You can't add properties to a non-extensible object, but you can change > >+ * attributes of properties in such objects. > > */ > >- if (sealed()) { > >+ if (!isExtensible()) { > > reportReadOnlyScope(cx); > > return NULL; > > } > > Jason caught this too -- this belongs in addPropertyCommon if you can delay > checking till then without issue (should be able to). Otherwise the > putProperty->addPropertyCommon case is not covered. > This should be named xml_fix now, here and below. Done. > >diff --git a/js/src/methodjit/PolyIC.cpp b/js/src/methodjit/PolyIC.cpp > >--- a/js/src/methodjit/PolyIC.cpp > >+++ b/js/src/methodjit/PolyIC.cpp > >@@ -559,8 +559,6 @@ class SetPropCompiler : public PICStubCo > > return disable("dense array"); > > if (!obj->isNative()) > > return disable("non-native"); > >- if (obj->sealed()) > >- return disable("sealed"); > > > > Class *clasp = obj->getClass(); > > > >@@ -590,8 +588,6 @@ class SetPropCompiler : public PICStubCo > > > > if (!holder->isNative()) > > return disable("non-native holder"); > >- if (holder->sealed()) > >- return disable("sealed holder"); > > > > if (!shape->writable()) > > return disable("readonly"); > > Don't we need some kind of !holder->isExtensible() check in the methodjit? This is a can of worms. I think the right thing is to check if obj->putProperty succeeded in adding the property (it can throw a TypeError or fail silently, in lenient mode code), and disable on that basis. But I think putProperty needs to take a 'strict' parameter, and have some way of indicating that it declined to add the property.

I need to sort through some error-signalling questions, but I should be able to turn this out pretty soon.

(In reply to comment #29) > > >+++ b/js/src/jsscope.cpp > > >@@ -721,12 +721,10 @@ JSObject::addProperty(JSContext *cx, jsi > > > JS_ASSERT(!JSID_IS_VOID(id)); > > > > > > /* > > >- * You can't add properties to a sealed object. But note well that you can > > >- * change property attributes in a sealed object, even though that replaces > > >- * a Shape * in the scope's hash table -- but no id is added, so the object > > >- * remains sealed. > > >+ * You can't add properties to a non-extensible object, but you can change > > >+ * attributes of properties in such objects. > > > */ > > >- if (sealed()) { > > >+ if (!isExtensible()) { > > > reportReadOnlyScope(cx); > > > return NULL; > > > } > > > > Jason caught this too -- this belongs in addPropertyCommon if you can delay > > checking till then without issue (should be able to). Otherwise the > > putProperty->addPropertyCommon case is not covered. This can be moved, assuming it's not a problem that JSObject::nativeSearch -> js::Shape::search -> js::PropertyTable::search will mark table slots with the COLLISION bit and then never install the property in the error case.

(In reply to comment #25) > Here's JSObject::preventExtensions for reference. Jason raised the question of > extensibleShapeChange with respect to attribute changes in freeze and seal. > It's a good point: generateOwnShape done via this code called from the top of > freeze or seal, followed by a loop calling js_SetAttributes, will regenerate > the |this| object's shape unnecessarily. Better to do the preventExtensions > call last, not first, in freeze and seal, if that doesn't break anything (it > shouldn't). Actually, as we walk the properties changing their attributes, the first change will put the object in dictionary mode, and the object will subsequently own the shape. Am I misunderstanding the concern here? If we do need to do preventExtensions last, then we need to rethink having the 'fix' hook supply the list of properties, as we'll need those before we do the fixing.

Revised per my latest understanding.

(In reply to comment #29) > An object may be non-extensible, but not have had its properties marked > non-configurable. In that case, even though !isExtensible, we still need to > enumerate the properties for the loop below to traverse. My point is that the only way an object becomes non-extensible is via JSObject::preventExtensions, which already does GetPropertyNames(<all of 'em>). There can't be any more to "get" here, and if there were, they would fail to be added because the object is already non-extensible. > > Here's JSObject::preventExtensions for reference. Jason raised the question of > > extensibleShapeChange with respect to attribute changes in freeze and seal. > > It's a good point: generateOwnShape done via this code called from the top of > > freeze or seal, followed by a loop calling js_SetAttributes, will regenerate > > the |this| object's shape unnecessarily. Better to do the preventExtensions > > call last, not first, in freeze and seal, if that doesn't break anything (it > > shouldn't). > > I didn't see how this will help; What I proposed will avoid giving the object OWN_SHAPE. > Jason will be commenting himself, concurring. For the single-shape case there's no dictionary mode. JSObject::changeProperty changes the last property referenced by the object, growing the shape tree. Jason's right that an object with more than one property will transition to dictionary mode, but it *still* pays to set OWN_SHAPE (via generateOwnShape, via extensibleShapeChange) after the setAttributes loop, not before. /be

> What I proposed will avoid giving the object OWN_SHAPE. ... before reshaping due to the setAttributes, of course. (In reply to comment #28) > You can optimize harder, and to freeze/seal a native object, > > - put the object into dictionary mode, giving it its own shapeid, This is not a clear description, because a dictionary does not have OWN_SHAPE even though it has unique shapes. OWN_SHAPE means the object has an objShape member value regenerated independent of lastProp->shape, on each shape change for any reason (see JSObject::updateShape). Rather than violate layering and make freeze and seal know about dictionary mode, I think it is better simply to reorder so the extensibleShapeChange call comes after the loop. > - clear the extensible bit, Of course it's "set the non-extensible bit". > - walk the linked list of Shapes and mutate the bits as desired. This is possibly faster, but only if in dictionary mode (more than one shape). Does not seem worth optimizing at the expense of preserving the layering and staying abstract w.r.t. dictionary mode and the mutable unshared shapes it connotes. More important, you cannot assume dictionary shapes are not shared -- Call objects may share frozen dictionary shapes with the callee function. Truly, it would be strange to freeze or seal a Call object, but this argues against the layering violation, for the sake of future-proofing. > This only uses up one shapeid, and it's a small amount of code -- but I'm not > sure this is what Brendan's proposing. It's not. I thought I was clear enough! Put the extensibleShapeChange call *after* the setAttributes loop. /be

(In reply to comment #32) > (In reply to comment #25) > Actually, as we walk the properties changing their attributes, the first change > will put the object in dictionary mode, and the object will subsequently own > the shape. Am I misunderstanding the concern here? Yes. Dictionary mode makes unique shapes but by itself does not generate yet another shape id for each mutation of the dictionary-mode object. Also, as noted, a single-property object won't go to dictionary mode. But this points out a bug where JSObject::changeProperty on a dictionary-mode object needlessly, via JSObject::udpateShape called after removing the old shape and adding the new one at lastProp, will regenerate objShape if the OWN_SHAPE flag is set. I'll fix that, but it's a separate issue. The main thing I was arguing for here is ordering to avoid setting OWN_SHAPE first. > If we do need to do preventExtensions last, then we need to rethink having the > 'fix' hook supply the list of properties, as we'll need those before we do the > fixing. Sorry, I should have seen this -- I was splitting out the extensibleShapeChange call in comments here. In this light, Jason's layering violation is better than splitting up the parts of preventExtensions. It's also faster (not that we need to optimize yet), but beware the single-shape and frozen (shared) dictionary shape cases! Layering the abstractions hides these details. Violating the layering means freeze and seal will have to check and take care. I'll review now and think about this more before typing too much. /be

(In reply to comment #31) > This can be moved, assuming it's not a problem that JSObject::nativeSearch -> > js::Shape::search -> js::PropertyTable::search will mark table slots with the > COLLISION bit and then never install the property in the error case. That will make for more work removing from the hashtable later, but it is minor until proven otherwise. OTOH, if earlier checking in two places is easier to understand and maintain, we can do that now and not worry about the premature collision flagging. /be

Comment on attachment 476180 [details] [diff] [review] Implement Object.preventExtensions, Object.isExtensible >+JSObject::sealOrFreeze(JSContext *cx, uintN sufficient_attrs, uintN new_attrs) >+{ >+ assertSameCompartment(cx, this); >+ JS_ASSERT(!isDenseArray()); >+ >+ AutoIdVector props(cx); >+ if (isExtensible()) { >+ if (!preventExtensions(cx, props)) >+ return false; >+ } else { >+ if (!GetPropertyNames(cx, this, JSITER_HIDDEN | JSITER_OWNONLY, props)) >+ return false; >+ } Again, if this object is not extensible, you can't possibly extend it with any missing lazy properties, and what's more, when it became non-extensible (by funneling through preventExtensions), GetPropertyNames(HIDDEN|OWNONLY) already happened. Objects get one chance only to reify lazy props before becoming non-extensible. So the else clause should go. >+ if ((attrs & JSPROP_PERMANENT) && >+ ((!sufficient_attrs) || (attrs & sufficient_attrs))) >+ continue; >+ Nit: no parens around !sufficient_attrs. Nit: brace consequent due to multiline condition. Non-nit: this is not equivalent to the check in Waldo's patch for freeze: if ((attrs & JSPROP_PERMANENT) && (!(attrs & (JSPROP_GETTER | JSPROP_SETTER)) || (attrs & JSPROP_READONLY))) { continue; That's "not an accessor or read-only", not "either an accessor or read-only". We don't want freeze to set JSPROP_READONLY (ES5 terms, set the [[Writable]] attribute to false) unless the property is a Data property: 15.2.3.9 Object.freeze ( O ) When the freeze function is called, the following steps are taken: 1. If Type(O) is not Object throw a TypeError exception. 2. For each named own property name P of O, a. Let desc be the result of calling the [[GetOwnProperty]] internal method of O with P. b. If IsDataDescriptor(desc) is true, then i. If desc.[[Writable]] is true, set desc.[[Writable]] to false. c. If desc.[[Configurable]] is true, set desc.[[Configurable]] to false. . . . >+ attrs |= JSPROP_PERMANENT | new_attrs; >+ if (!setAttributes(cx, id, &attrs)) >+ return false; >+ } Glad to see the layering (setAttributes) preserved instead of mixing in knowledge of dictionary-mode objects in detail. Don't worry about my OWN_SHAPE issue, I'll fix it separately -- sorry for injecting it here. /be

Ignore me, I'm an idiot -- you need that else clause calling GetPropertyNames just to get the property names. Sorry for that noise. /be

I noted the OWN_SHAPE dictionary-mode object gratuitous shape regeneration issues along with a frozen issue at bug 593129 comment 5. Jason, if you could review my comment there, that would be helpful. Sorry for dragging you down the layering violation rabbit hole, it shouldn't hold up the progress toward patching this bug (but it was helpful anyway). /be

One point I raised earlier, re: the design of fix, was that objects should not have to throw to refuse to be fixed, at least not in the proxy design. We can do it in a separate bug, but I wonder if here and now is the right place and time to settle fix's API. If fix returned another out param saying "I refuse", we could throw in preventExtensions. The proxy handler's fix trap is going to do that by returning undefined. This could be done in jsproxy.cpp, but then all the fix implemntations (as in the patches here) have to replicate the throw. Small design issue, thought I'd re-raise it since it may have been missed. /be

> That's "not an accessor or read-only", not "either an accessor or read-only". Tricky of me to bind "not" tigher than "or" -- a comma would have helped: > That's "not an accessor, or read-only", not "either an accessor or read-only". Hope this ES5 conformance catch makes up for my atrocious comment spam! /be

(In reply to comment #42) > Hope this ES5 conformance catch makes up for my atrocious comment spam! Nope. I'm gonna shut up and sleep now. /be

Comment on attachment 476180 [details] [diff] [review] Implement Object.preventExtensions, Object.isExtensible >+ if ((attrs & JSPROP_PERMANENT) && >+ ((!sufficient_attrs) || (attrs & sufficient_attrs))) >+ continue; Nit: don't over-parenthesize !sufficient_attrs, do brace if you keep the multiline condition (but it might fit on one line within 100 columns). r=me, finally -- I actually almost fell asleep and then my misreading of the logic came to me. /be

Forgot to suggest making sealOrFreeze private in JSObject. /be

(In reply to comment #38) > >+ if ((attrs & JSPROP_PERMANENT) && > >+ ((!sufficient_attrs) || (attrs & sufficient_attrs))) > >+ continue; > >+ > > Nit: no parens around !sufficient_attrs. Fixed. > Nit: brace consequent due to multiline condition. Fixed. > Non-nit: this is not equivalent to the check in Waldo's patch for freeze: > > if ((attrs & JSPROP_PERMANENT) && > (!(attrs & (JSPROP_GETTER | JSPROP_SETTER)) || (attrs & > JSPROP_READONLY))) { > continue; > > That's "not an accessor or read-only", not "either an accessor or read-only". > > We don't want freeze to set JSPROP_READONLY (ES5 terms, set the [[Writable]] > attribute to false) unless the property is a Data property: There was a comment (that I accidentally removed when I common-ified the two loop bodies) saying that it was harmless to set JSPROP_READONLY on accessor properties. But let me see if I can re-hack this.

(In reply to comment #45) > Forgot to suggest making sealOrFreeze private in JSObject. Good idea; done.

(In reply to comment #41) > One point I raised earlier, re: the design of fix, was that objects should not > have to throw to refuse to be fixed, at least not in the proxy design. We can > do it in a separate bug, but I wonder if here and now is the right place and > time to settle fix's API. > > If fix returned another out param saying "I refuse", we could throw in > preventExtensions. The proxy handler's fix trap is going to do that by > returning undefined. This could be done in jsproxy.cpp, but then all the fix > implemntations (as in the patches here) have to replicate the throw. > > Small design issue, thought I'd re-raise it since it may have been missed. I've added a |bool *fixed| outparam to the 'fix' method.

Note bug 598225. Should JS_FreezeObject make shared+permanent properties read-only as well?

Perhaps this was understood in the initial reviews, but there are some changes in the semantics of the old JS_SealObject and the new JS_FreezeObject: - Old JS_SealObject made the entire object read-only with a single flag checked at the top of js_SetPropertyHelper. Thus, the SEALED flag affected slotless properties inherited from prototypes, inherited setters, and so on. - New JS_FreezeObject marks the object as NOT_EXTENSIBLE, and marks each own property as read-only. The NOT_EXTENSIBLE flag is checked later in js_SetPropertyHelper, where we've determined we're going to add a property to the object. Inherited setters and slotless properties still run. Also, the SEALED flag always threw an error when it prevented an access, whereas NOT_EXTENSIBLE (per ES5) fails silently in non-strict mode code. This affects CTypes tests, which expect errors to be thrown, and expects JS_SealObject to make function pointer CData objects be read-only (|value| is an inherited slotless property). There is also code in js/src/xpconnect/src that used JS_SealObject and now uses JS_FreezeObject, including XPCNativeWrapper. We need to think that through very carefully.

The only places we're using JS_SealObject in xpconnect are XPCNativeWrapper::AttachNewConsructorObject (to seal XPCNativeWrapper.prototype) and XPCSafeJSObjectWrapper::AttachNewConstructorObject (to seal XPCSafeJSObjectWrapper.prototype). I don't think they have any properties except data properties. And I think their prototype is null, and I don't think there's any way to set it. If that's true, this is not a security/integrity issue. mrbkap can check me.

Yes, the JS_SealObject API removal is a real break, and JS_FreezeObject is not a perfect replacement. This is a calculated move. We want ES5 semantics, without old extensions to ES1 if possible. Can the CTypes tests be adjusted? /be

Rhino had s (pre-ES1?). Cc'ing Norris. /be

(In reply to comment #53) > Rhino had s (pre-ES1?) er, "seal" API, too. /be

https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/Object/preventExtensions https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/Object/isExtensible