Last Comment Bug 492849 - ES5: Implement Object.preventExtensions, Object.isExtensible
: ES5: Implement Object.preventExtensions, Object.isExtensible
Status: RESOLVED FIXED
[fixed-in-tracemonkey]
: dev-doc-complete
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Jim Blandy :jimb
:
Mentors:
: 541212 (view as bug list)
Depends on: 598225 558451 597574 598969 599459 601397
Blocks: 430133 es5 harmony:proxies 595022
  Show dependency treegraph
 
Reported: 2009-05-13 13:49 PDT by Jim Blandy :jimb
Modified: 2010-10-04 16:11 PDT (History)
27 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
beta7+


Attachments
Patch, not sure whether property-set behavior after O.pE (throws with this patch) is to-spec or not (3.47 KB, patch)
2009-09-04 14:45 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
no flags Details | Diff | Splinter Review
Partial patch, dependent on tvr refactoring, AutoValueArray, and getOwnPropertyNames (19.84 KB, patch)
2010-03-05 16:16 PST, Jeff Walden [:Waldo] (remove +bmo to email)
no flags Details | Diff | Splinter Review
Updated, atop Brendan's newly-landed work (37.37 KB, patch)
2010-08-31 12:52 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
no flags Details | Diff | Splinter Review
Rebased (39.07 KB, patch)
2010-09-09 15:34 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
no flags Details | Diff | Splinter Review
Partially updated for comments (38.79 KB, patch)
2010-09-10 01:46 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
no flags Details | Diff | Splinter Review
Implement Object.preventExtensions, Object.isExtensible. (41.24 KB, patch)
2010-09-16 11:48 PDT, Jim Blandy :jimb
no flags Details | Diff | Splinter Review
Implement Object.preventExtensions, Object.isExtensible (41.21 KB, patch)
2010-09-16 22:53 PDT, Jim Blandy :jimb
brendan: review+
Details | Diff | Splinter Review

Description Jim Blandy :jimb 2009-05-13 13:49:53 PDT
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
Comment 1 Jeff Walden [:Waldo] (remove +bmo to email) 2009-09-04 14:45:04 PDT
Created attachment 398764 [details] [diff] [review]
Patch, not sure whether property-set behavior after O.pE (throws with this patch) is to-spec or not
Comment 2 Jeff Walden [:Waldo] (remove +bmo to email) 2009-09-09 13:58:37 PDT
Per ES5, property-adds should silently fail; property-changes (including to/from function values, sigh) should work.  This patch does neither.
Comment 3 Andreas Gal :gal 2010-02-18 11:23:20 PST
Waldo, whats the status of this? Any progress?
Comment 4 Jeff Walden [:Waldo] (remove +bmo to email) 2010-02-18 12:19:33 PST
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.
Comment 5 Jeff Walden [:Waldo] (remove +bmo to email) 2010-03-05 16:16:47 PST
Created attachment 430758 [details] [diff] [review]
Partial patch, dependent on tvr refactoring, AutoValueArray, and getOwnPropertyNames
Comment 6 Jeff Walden [:Waldo] (remove +bmo to email) 2010-07-25 21:02:01 PDT
http://wiki.whatwg.org/wiki/Web_ECMAScript#Object_Properties points out that when this is implemented, it should also kill __proto__-changes.
Comment 7 Mark S. Miller 2010-07-26 17:17:41 PDT
Yes. ES5 points this out too ;).
Comment 8 Jeff Walden [:Waldo] (remove +bmo to email) 2010-08-18 15:25:22 PDT
This bug depends on Brendan's scope-removal work in bug 558451.
Comment 9 Brendan Eich [:brendan] 2010-08-18 16:07:49 PDT
(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
Comment 10 Jeff Walden [:Waldo] (remove +bmo to email) 2010-08-25 14:23:40 PDT
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.
Comment 11 Brendan Eich [:brendan] 2010-08-26 21:53:27 PDT
(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
Comment 12 Jeff Walden [:Waldo] (remove +bmo to email) 2010-08-31 12:52:42 PDT
Created attachment 470865 [details] [diff] [review]
Updated, atop Brendan's newly-landed work

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 13 Jeff Walden [:Waldo] (remove +bmo to email) 2010-09-09 15:34:14 PDT
Created attachment 473777 [details] [diff] [review]
Rebased
Comment 14 Brendan Eich [:brendan] 2010-09-09 16:22:53 PDT
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!
Comment 15 Brendan Eich [:brendan] 2010-09-09 17:44:37 PDT
(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
Comment 16 Jeff Walden [:Waldo] (remove +bmo to email) 2010-09-09 17:48:19 PDT
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...
Comment 17 Brendan Eich [:brendan] 2010-09-09 17:57:12 PDT
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
Comment 18 Jeff Walden [:Waldo] (remove +bmo to email) 2010-09-10 01:46:47 PDT
Created attachment 473991 [details] [diff] [review]
Partially updated for comments

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.
Comment 19 Jeff Walden [:Waldo] (remove +bmo to email) 2010-09-10 01:47:46 PDT
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.
Comment 20 Brendan Eich [:brendan] 2010-09-10 15:10:03 PDT
(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
Comment 21 Robert Sayre 2010-09-10 15:21:16 PDT
jimb will take this over the finish line.
Comment 22 Mark S. Miller 2010-09-11 21:24:18 PDT
> 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?
Comment 23 Jim Blandy :jimb 2010-09-16 11:48:41 PDT
Created attachment 475931 [details] [diff] [review]
Implement Object.preventExtensions, Object.isExtensible.
Comment 24 Jim Blandy :jimb 2010-09-16 11:50:46 PDT
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 25 Brendan Eich [:brendan] 2010-09-16 14:56:44 PDT
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
Comment 26 Brendan Eich [:brendan] 2010-09-16 15:01:40 PDT
(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
Comment 27 Jason Orendorff [:jorendorff] 2010-09-16 16:08:35 PDT
(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().
Comment 28 Jason Orendorff [:jorendorff] 2010-09-16 16:15:55 PDT
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.
Comment 29 Jim Blandy :jimb 2010-09-16 16:52:16 PDT
(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.
Comment 30 Jim Blandy :jimb 2010-09-16 17:34:09 PDT
I need to sort through some error-signalling questions, but I should be able to turn this out pretty soon.
Comment 31 Jim Blandy :jimb 2010-09-16 20:55:35 PDT
(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.
Comment 32 Jim Blandy :jimb 2010-09-16 22:50:22 PDT
(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.
Comment 33 Jim Blandy :jimb 2010-09-16 22:53:23 PDT
Created attachment 476180 [details] [diff] [review]
Implement Object.preventExtensions, Object.isExtensible

Revised per my latest understanding.
Comment 34 Brendan Eich [:brendan] 2010-09-16 23:55:01 PDT
(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
Comment 35 Brendan Eich [:brendan] 2010-09-17 00:03:31 PDT
> 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
Comment 36 Brendan Eich [:brendan] 2010-09-17 00:15:08 PDT
(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
Comment 37 Brendan Eich [:brendan] 2010-09-17 00:18:10 PDT
(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 38 Brendan Eich [:brendan] 2010-09-17 00:35:21 PDT
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
Comment 39 Brendan Eich [:brendan] 2010-09-17 00:44:18 PDT
Ignore me, I'm an idiot -- you need that else clause calling GetPropertyNames just to get the property names. Sorry for that noise.

/be
Comment 40 Brendan Eich [:brendan] 2010-09-17 00:58:46 PDT
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
Comment 41 Brendan Eich [:brendan] 2010-09-17 01:02:53 PDT
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
Comment 42 Brendan Eich [:brendan] 2010-09-17 01:05:10 PDT
> 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
Comment 43 Brendan Eich [:brendan] 2010-09-17 03:06:46 PDT
(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 44 Brendan Eich [:brendan] 2010-09-17 03:10:42 PDT
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
Comment 45 Brendan Eich [:brendan] 2010-09-17 08:08:34 PDT
Forgot to suggest making sealOrFreeze private in JSObject.

/be
Comment 46 Jim Blandy :jimb 2010-09-17 14:29:04 PDT
(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.
Comment 47 Jim Blandy :jimb 2010-09-17 15:11:25 PDT
(In reply to comment #45)
> Forgot to suggest making sealOrFreeze private in JSObject.

Good idea; done.
Comment 48 Jim Blandy :jimb 2010-09-17 15:54:46 PDT
(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.
Comment 49 Jim Blandy :jimb 2010-09-20 18:17:46 PDT
Note bug 598225.  Should JS_FreezeObject make shared+permanent properties read-only as well?
Comment 50 Jim Blandy :jimb 2010-09-21 08:04:56 PDT
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.
Comment 51 Jason Orendorff [:jorendorff] 2010-09-21 09:59:02 PDT
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.
Comment 52 Brendan Eich [:brendan] 2010-09-21 10:31:45 PDT
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
Comment 53 Brendan Eich [:brendan] 2010-09-21 10:32:21 PDT
Rhino had s (pre-ES1?). Cc'ing Norris.

/be
Comment 54 Brendan Eich [:brendan] 2010-09-21 10:36:30 PDT
(In reply to comment #53)
> Rhino had s (pre-ES1?)

er, "seal" API, too.

/be
Comment 55 Jim Blandy :jimb 2010-09-21 11:41:44 PDT
http://hg.mozilla.org/tracemonkey/rev/aa9b86572020
Comment 56 Robert Sayre 2010-09-21 11:55:15 PDT
*** Bug 541212 has been marked as a duplicate of this bug. ***

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