518663 - ES5: Object.getOwnPropertyNames

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Comment 1

[Jeff Walden [:Waldo]]

The big problem is that there's no API to determine the non-enumerable properties of an object.  They don't have to show up until they're directly resolved, and there are no hooks responsible for defining them eagerly.  It seems to me that we need some sort of API for objects to define their un-enumerable properties or to iterate through the names of all properties.

In any case, this is future-fodder, not happening now.

Comment 2

[Jeff Walden [:Waldo]]

Attachment: Pure-sodium strawman (atop bug 536890 and maybe other changes)

It's time to have the argument about binary compatibility.

I see no way to support Object.getOwnPropertyNames on arbitrary objects without adding a new hook to JSClass to do so.  Since resolve hooks can add arbitrary properties without the engine having any knowledge of such, anything using existing APIs will miss those properties.  We also need a hook to implement the object immutability APIs; since even the least of those methods, Object.preventExtensions, stops new properties from being added to the object, we need a way to say "add/reveal all implicit properties now, or forever hold your peace" before marking the object as no longer extensible.

We've mostly skirted making backwards-incompatible changes to the JSAPI for awhile, even as we make small changes here and there.  But we've never promised binary compatibility, only source compatibility.  This strawman patch would permit source compatibility, at the cost of Object.getOwnPropertyNames being a lie.  It is incomplete: results for various classes not js_ObjectClass with hidden, resolve-only properties are still wrong.  It would be good to share the code of js_Enumerate somehow, although at the moment I'm not sure quite how -- maybe fun_enumerateAll that tail-calls js_Enumerate and in the init case resolves all properties on the function (mutatis mutandis for the others, I've only just reached the issue and not given it thought).  I'm reluctant to spend too much more time on a strawman without settling the fundamental arguments.

But anyway: I don't think we can implement this feature or the object immutability features correctly without JSAPI changes.  I believe it is more important to implement a new spec than to half-preserve backwards compatibility.  We are being held back by this, while other engines blindly forge ahead with non-existent compatibility stories.  They are willing to make their users run with them to keep working.  It's nice that we've mostly been better about that, but do we really sacrifice features for hundreds of millions of users for the far fewer users embedding SpiderMonkey?  Do we sacrifice the ability to keep our code clean (JSClass::enumerate [JSEnumerateOp edition], JSClass::enumerate [JSNewEnumerateOp edition], and JSObjectOps::enumerate, I'm looking at you) and instead pile on more and more shims as we go?  I don't think we should.

With that, flame away.

Comment 3

[Brendan Eich [:brendan]]

mrbkap and jorendorff should weigh in. I'm fine with combining JSExtendedClass and JSClass, or changing JSClass.enumerate (again -- could even do it compatibly with JSCLASS_NEWER_ENUMERATE ;-). All possible. Things to avoid:

* exposing too much in the public API that depends on internals we want to change (last time we talked about this, in bug 408416, we thought we could make JSClass some kind of "recipe" for an internal, flat ops vector).

* burdening all classes with something marginal that only a few need (ahem, XDR).

* making it hard for binary add-ons to detect that they have a new API and their use of it via old entry points might crash; dunno if we have a good solution here or if it would be used, since add-ons have to announce their compatibility (maxVersion).

/be

Comment 4

[Brendan Eich [:brendan]]

Extending JSClass.enumerate (JSNewEnumerateOp flavor) seems quite easy, AFAICT, and I agree with Waldo's f2f point that it would be the best way to avoid code duplication. Just give it an "I want all properties, not just enumerable ones" advice-parameter when starting enumeration.

/be

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 425607 [details] [diff] [review]
Pure-sodium strawman (atop bug 536890 and maybe other changes)

>+MSG_DEF(JSMSG_NONNATIVE_GETOWNPROPERTYNAMES, 247, 0, JSEXN_INTERNALERR, "can't get own property names for the provided object")

Please to be keeping messages shorter, avoiding deadwood such as "the provided object" -- if you want to say something, cite the decompiled expression that resulted in the offending object reference.

Also, the JSMSG_* name length outliers really out-lie too much:

$ sed -n 's/.*JSMSG_\([A-Za-z0-9_]*\).*/\1/p' js.msg|awk '{if (length($1)>30) a[i++] = $1} END {for (i in a) print(a[i])}'
DEFINE_ARRAY_LENGTH_UNSUPPORTED
CANT_REDEFINE_UNCONFIGURABLE_PROP
CANT_APPEND_PROPERTIES_TO_UNWRITABLE_LENGTH_ARRAY

Suggest shortening at least the last one, plus the one this patch adds (not yet used?), to in-lie.

>diff --git a/js/src/jsapi.h b/js/src/jsapi.h
>--- a/js/src/jsapi.h
>+++ b/js/src/jsapi.h
>@@ -1432,6 +1432,7 @@ struct JSClass {
>     JSHasInstanceOp     hasInstance;
>     JSMarkOp            mark;
>     JSReserveSlotsOp    reserveSlots;
>+    JSEnumerateAllOp    enumerateAll;

Instead of adding yet another JSClass hook (extended class, doesn't matter), why not change JSNewEnumerateOp? Then the code sharing would be greater, no need for separate hook impls with common code factored into subroutine(s). From jspubtd.h:

/*
 * This enum type is used to control the behavior of a JSObject property
 * iterator function that has type JSNewEnumerate.
 */
typedef enum JSIterateOp {
    JSENUMERATE_INIT,       /* Create new iterator state */
    JSENUMERATE_NEXT,       /* Iterate once */
    JSENUMERATE_DESTROY     /* Destroy iterator state */
} JSIterateOp;

. . . 

typedef JSBool
(* JSNewEnumerateOp)(JSContext *cx, JSObject *obj, JSIterateOp enum_op,
                     jsval *statep, jsid *idp);

Add JSENUMERATE_ALL_INIT, and cooperating implementations would have to keep a bit telling them not to filter on JSPROP_ENUMERATE.

The code size increase in this patch is just not required in order to implement ES5 getOwnPropertyNames.

/be

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 425607 [details] [diff] [review]
Pure-sodium strawman (atop bug 536890 and maybe other changes)

Also, patching js_EnumerateAll into js_ObjectClass and js_SlowArrayClass is not enough. We need getOwnPropertyNames to work on all classes of objects, and class hooks are not delegated up the prototype chain (property gets and sets happen to work as if the class getter or setter is delegated, in the absence of a per-property getter or setter, but this is just via defaulting).

The JSENUMERATE_ALL_INIT JSNewEnumerateOp evolution applies to JSObjectOps and JSClass, and the js_Enumerate impl for native object ops would be the place to handle slow arrays, capital-O objects, and all other native objects.

Non-native object-ops are gone now, IIRC. The non-js_ObjectOps impls all copy js_ObjectOps (the native object ops) and selectively override. Check me on this point.

/be

Comment 7

[Jeff Walden [:Waldo]]

getOwnPropertyNames is, as the name indicates, own properties.  It doesn't need to delegate up the prototype chain (although permanent/shared properties on prototypes need to be addressed somehow -- most of the hooking in the patch was to work around that botch).

It does make sense to change the enumerate hook; the problems of having two different enumeration methods (and syncing between the two) were becoming apparent when I last touched this a few days ago.

Comment 8

[Brendan Eich [:brendan]]

(In reply to comment #7)
> getOwnPropertyNames is, as the name indicates, own properties.  It doesn't need
> to delegate up the prototype chain (although permanent/shared properties on
> prototypes need to be addressed somehow -- most of the hooking in the patch was
> to work around that botch).

Yes, I know. But did you know that JSObjectOps.enumerate does not go up the proto chain either?

You are hacking at the wrong level. You haven't changed js_FunctionClass, js_DateClass, etc. etc. Nor should you: property mapping is done by object-ops impls, mainly the "native" one that uses JSScope. So hacking a new class hook is wrong even ignoring code bloat.

> It does make sense to change the enumerate hook; the problems of having two
> different enumeration methods (and syncing between the two) were becoming
> apparent when I last touched this a few days ago.

What problems?

/be

Comment 9

[Jeff Walden [:Waldo]]

(In reply to comment #8)
> > It does make sense to change the enumerate hook; the problems of having two
> > different enumeration methods (and syncing between the two) were becoming
> > apparent when I last touched this a few days ago.
> 
> What problems?

Mostly the chance of the two becoming unsynchronized and exposing property names in different orders.

Comment 10

[Brendan Eich [:brendan]]

(In reply to comment #9)
> (In reply to comment #8)
> > > It does make sense to change the enumerate hook; the problems of having two
> > > different enumeration methods (and syncing between the two) were becoming
> > > apparent when I last touched this a few days ago.
> > 
> > What problems?
> 
> Mostly the chance of the two becoming unsynchronized and exposing property
> names in different orders.

I am suggesting precisely *not* duplicating js_Enumerate into js_EnumerateAll and tweaking to reflect all, not just enumerable, property ids. Rather, add a JSENUMERATE_ALL_INIT enumerator to the JSIterateOp enum and implement it in js_Enumerate with minimal added logic to test the bit in the iterator and either skip non-enumerables, or (if the ALL bit is set) include them.

It's your patch that is duplicative, and that's one of three objections. The other two are wrong-level and incompleteness (related, not the same).

Ok, what am I missing?

/be

Comment 11

[Brendan Eich [:brendan]]

FWIW, the JSObjectOps/JSClass history and layering is analogous to filesystem switch (vnode) vs. device driver (bdevsw/cdevsw in old Unix) kernel layering. At first there was only one filesystem implementation, but many (block not char, for filesystems) device drivers.

The same was true originally with the jsobj.c(pp) js_* functions now plugged into js_ObjectOps, which had many JSClass variations dispatched "under" them. With LiveConnect and Java's different semantics (class metadata instead of JSScope; overloading and other name-lookup differences; no "expandos" allowed), we virtualized the upper js_* layer. This was in 1997 or 1998, I forget when exactly. It had the odd effect of delegating getObjectOps to JSClass.

Per bug 408416, we might flatten everything to a new single ops vector, private to the VM, relegating JSClass to the role of identity singleton and initializer or "ops recipe" -- but then to avoid duplicate code we would plug the same function pointers into many of the native class implementation's vectors.

Still might be a win. Cleaner for sure, if only by virtue of losing the delegated JSClass.getObjectOps optional hook. But this is all not the point, not necessary to fix this bug.

What's needed here is a tweak to JSIterateOp and the JSNewEnumerateOp types and implementations, to conserve the "own" property traversal code, but optionally no longer filter out non-enumerable own properties. This is straightforward to do.

/be

Comment 12

[Brendan Eich [:brendan]]

BTW, fur's JSIterateOp name is infelicitous, now in light of iterators, even then in adding another name for enumerators. It's really JSEnumerateCommand or some such (JSNewEnumerateCommand? Bleah). Could fix with a compatibility typedef-alias.

/be

Comment 13

[Jeff Walden [:Waldo]]

Mixing permanent/shared properties on class prototypes into enumeration hooks isn't simple; mixing it in beside the existing enumeration APIs is even worse.  I'm strongly inclined, and jorendorff supports, removing shared usage in the engine as a crutch to save duplicating a few ES-standard properties in this manner.  Non-engine code will simply not get such names listed in output; this seems like a good way to nudge embedders towards deprecation of the shared/permanent trick as a way of introducing instance properties.

This is a prerequisite which will go in another bug -- my current WIP is big enough as-is without folding in this extra bit.  Keeping it separate also permits benchmarking of the cost of this choice -- a necessary one, I think, and one whose simplification value makes it worthwhile regardless, but still one whose footprint effects we should know.

Comment 14

[Brendan Eich [:brendan]]

Less attitude about non-standard features, more measurement, please -- you are going to create a direct ("own") 'length' property in every function object in the engine, requiring a mutable scope on every function object in the engine, including every cloned function object? That's not taking a way a crutch, it is cutting off your nose to spite your face!

Hard cases can pay, the common case should not. It would be better to leave out shared/permanent enumerate-all results for a first cut and then we can see how to enumerate any that exist.

/be

Comment 15

[Brendan Eich [:brendan]]

Waldo explained in person that he was thinking of using resolve to bind direct properties on demand. That is pretty much bound to be ok for function lengths (which are arities, bleargh), since they are rarely used.

Array lengths are also shared permanent, but much more often used than function lengths. Good news is that we have a JSOP_LENGTH fast path for getting the length of a dense array (named property accesses bypass dense arrays otherwise). So this might be ok to change for slow arrays, but it could ding performance. Only way to tell is measure.

RegExp properties, at least lastIndex, are more often used -- they're also currently implemented via shared/permanents with getter and setter in common.

Another place that relies on shared/permanent: let bindings, where the compiler-created block object serves as the prototype of the runtime cloned block object, and for let performance we don't want sets to shadow the proto-property. Firefox XUL JS uses let a lot, and I've seen js_GetScopeChain clone blocks, so the set costs here could be noticeable.

Each of these could maybe be dealt with. Is that easier than enumerate-all going up the proto chain in search of shared/permanents? I suspect it's not, but let's find out in a separate bug, and get enumerate-all going without worrying about shared permanents.

/be

Comment 16

[Jeff Walden [:Waldo]]

Attachment: WIP, without shared-permanents

I killed all the extra bits to make shared-permanent names show up, so this is stupid and minimally/incorrectly working.

Comment 17

[Jeff Walden [:Waldo]]

Making good progress on this now, think I have the JS engine bits fixed now and am just working through all in-tree users.  It's curious just how many seem to enumerate non-enumerable properties.  :-)

Comment 18

[Jeff Walden [:Waldo]]

Attachment: Patch, v1

Comment 19

[Brendan Eich [:brendan]]

Comment on attachment 454098 [details] [diff] [review]
Patch, v1

>+         * FIXME: The shared-permanent hack makes own-property-only enumeration
>+         *        O(protoChainLength * propsPerProto), rather than simply
>+         *        O(propsOnObject) as one would expect.  In the short run we
>+         *        find it expedient to not spend the time to eliminate the
>+         *        hack; in the (hopefully sufficiently) long run this quadratic
>+         *        behavior will come back to bite us.

Do we have a bug to cite here? I see particular bugs such as bug 548671 and bug 565825, but not a "get rid of JSPROP_SHARED" or "change JSPROP_SHARED to JSPROP_SLOTLESS" bug. We should discuss another alternative: making getters and setters imply no-slot, otherwise you get a slot. No need for any attribute, frees an attr bit.

Please find or file and cite, in any case.

> {
>+    /*
>+     * FIXME: If it weren't for the shared-permanent hack (see Enumerate and
>+     *        js_HasOwnProperty), we could avoid initializing this hash table
>+     *        when (flags & JSITER_OWNONLY).

Cite again, briefly -- no drama queening :-P.

>+#if JS_HAS_OBJ_PROTO_PROP
>+    /*
>+     * We don't want __proto__ to show up during non-enumerable property
>+     * iteration; no developer will expect it, and from appearances all other
>+     * browsers expose __proto__ in a manner which causes it not to show up in
>+     * non-enumerable iteration.  Ideally we would remove the property
>+     * entirely, but doing so at the moment would Break the Web.  Exposing it
>+     * during iteration (even non-enumerable iteration only through
>+     * Object.getOwnPropertyNames) would make removal even more difficult
>+     * (someone would start using Object.getOwnPropertyNames(o).slice(1) or
>+     * Object.getOwnPropertyNames(o).slice(0, -1) to hack it off entirely).
>+     * Fortunately it is possible to remove the property from iteration without
>+     * removing it entirely.

This is well-reasoned but it should be condensed.

>+     * Marking a property as an alias property prevents it from showing up
>+     * during non-enumerable property iteration.  This is by design!  (Whether
>+     * the alias-property concept is truly necessary is a separate question.)

Separate bug.

Historical note: aliases were in JS from day 1, never made it into ES1. They were for document.forms[0] and document.forms.myFirstForm aliasing. We should try to get rid of them, I may have asked for a separate bug already. Anyone can file, I'm gonna let someone else (maybe the person most offended by aliases :-P) file.

>+     * Marking __proto__ as an alias property, then, will hide it from
>+     * enumeration.  The complication in marking __proto__ as such a property
>+     * is that it really isn't an alias property at all: there's no other
>+     * property whose exact behavior it emulates, as is the intent of alias
>+     * properties.  However, with a bit of care, we can still mark it as an
>+     * alias property to piggyback on their never-enumerated property.

Nit "their" has no referent (singular "alias property" is not in the right grammatical position for "of their" to reference).

>  Object
>+     * GC tracing code assumes that certain aspects of alias properties needn't
>+     * be examined, because the remaining portion of the property will be
>+     * traced when the aliased property is traced.  Those assumptions are not
>+     * generally valid, but because tracing an object necessarily traces its
>+     * [[Prototype]], we don't need to worry about this.
>+     *
>+     * FIXME: This code is among the most abominable I have ever written, but I
>+     *        do not believe it is possible to feasibly address this problem
>+     *        any other way right now.  Patches with code to do this more
>+     *        elegantly are more than welcome.

This is way overlong, drama-queening, and over-personalized yet unsigned. Cut it down to a FIXME citing a bug to fix if the following does not lead to a better short-ish term solution:

Did you consider simply excluding __proto__ (rt->atomState.protoAtom) directly in the jsiter.cpp code?

"When in doubt, use brute force."

/be

Comment 20

[Jason Orendorff [:jorendorff]]

This makes some properties enumerable that weren't before, e.g. in nsStorageSH, nsStorage2SH, nsHTMLFormElementSH. What's the rationale for that?

Comment 21

[Jeff Walden [:Waldo]]

The properties showed up if you did |for (var p in o)| on them.  It seems more likely people will have relied on their showing up in for-in loops than on their not appearing enumerable to propertyIsEnumerable or what-have-you.

Comment 22

[Jeff Walden [:Waldo]]

(In reply to comment #19)
> Do we have a bug to cite here?

No.  The problem is that shared-permanent implies own, and it doesn't have anything to do with slotfulness, or slotlessness, as far as I can tell.  I believe we don't think that's solvable until after scope removal, so I haven't given this any thought.


Not bothering with the comment-nitting, will wait for jorendorff's review to get to that..


> Did you consider simply excluding __proto__ (rt->atomState.protoAtom) directly
> in the jsiter.cpp code?

__proto__ is only special on Object.prototype, which would mean bloating all iteration over normal objects, even worse than the current constant bitmasking to exclude aliases.  I did consider a custom enumeration hook for Object.prototype (by giving it a different class?), but that seemed a good deal of additional pain just for this one case.  In the end piggybacking on existing behavior seemed a better option (not a good one, but a better one).

Comment 23

[Brendan Eich [:brendan]]

(In reply to comment #22)
> (In reply to comment #19)
> > Do we have a bug to cite here?
> 
> No.  The problem is that shared-permanent implies own, and it doesn't have
> anything to do with slotfulness, or slotlessness, as far as I can tell.

JSScope::getChildProperty(JSContext *cx, JSScopeProperty *parent,
                          JSScopeProperty &child)
{
    JS_ASSERT(!JSVAL_IS_NULL(child.id));
    JS_ASSERT(!child.inDictionary());

    /*
     * Aliases share another property's slot, passed in the |slot| parameter.
     * Shared properties have no slot. Unshared properties that do not alias
     * another property's slot allocate a slot here, but may lose it due to a
     * JS_ClearScope call.
     */
    if (!child.isAlias()) {
        if (child.attrs & JSPROP_SHARED) {
            child.slot = SPROP_INVALID_SLOT;
        } else {
            /*
             * We may have set slot from a nearly-matching sprop, above.
             * If so, we're overwriting that nearly-matching sprop, so we
             * can reuse its slot -- we don't need to allocate a new one.
             * Similarly, we use a specific slot if provided by the caller.
             */
            if (child.slot == SPROP_INVALID_SLOT &&
                !js_AllocSlot(cx, object, &child.slot)) {
                return NULL;
            }
        }
    }

JSPROP_SHARED was added to avoid a slot, since proto-properties with getters and setters *and* slots tend to entrain last-got or last-set value as garbage, which can be a link to a huge, otherwise-dead graph.

> I
> believe we don't think that's solvable until after scope removal, so I haven't
> given this any thought.

You don't have to give it thought to file a bug to cite in a FIXME, and anyway we have discussed removing JSPROP_SHARED, most recently here (previously in other bugs Jason is on). Filing a FIXME bug instead of taking the time to write a comment complaining about something is clearly the right thing. Wasting time writing the complaint is pretty much the wrong thing.

> > Did you consider simply excluding __proto__ (rt->atomState.protoAtom) directly
> > in the jsiter.cpp code?
> 
> __proto__ is only special on Object.prototype, which would mean bloating all
> iteration over normal objects,

No, this would go in existing the last-object-on-proto-chain case code. For completeness against ES5 Object.create(null, {__proto__: ...}) the code could also check for the magic getter, which is unforgeable.

> even worse than the current constant bitmasking
> to exclude aliases.

Why are you prematurely optimizing getOwnPropertyNames? Performance at this point does not matter, and a protoAtom check even if mispredicted is near-target and without profile data therefore not worth worrying about.

It beats hacking on aliases to hide __proto__, especially while at the same time complaining about, if not actually working on getting rid of, aliases. Why decry one thing while building on it?

> I did consider a custom enumeration hook for
> Object.prototype (by giving it a different class?)

That is a non-starter.

Stop prematurely optimizing a new-in-ES5 meta-object API to save a minor branch mispredict penalty.

/be

Comment 24

[Brendan Eich [:brendan]]

(In reply to comment #23)
> For
> completeness against ES5 Object.create(null, {__proto__: ...}) the code could
> also check for the magic getter, which is unforgeable.

Heh, that Object.create call doesn't work -- need

var o = Object.create(null);
o.__proto__ = "no the droid you're looking for";

/be

Comment 25

[Jeff Walden [:Waldo]]

Attachment: Patch, v1.1

(In reply to comment #23)

"What we have here is failure to communicate."
- http://www.youtube.com/watch?v=1fuDDqU6n4o

I really am trying.  I wish I knew what I manage to do consistently wrong that so often has one or the other of us jumping at the other's throat when philosophies differ.


My understanding of your desired approach to the shared-permanent problem is to rename shared to slotless and to use it in fewer situations, but for reasons of compatibility not remove it.  It doesn't seem to me that that solves the fundamental problem which Jason succinctly stated in bug 548671 comment 7:

"The effect of JSPROP_SHARED | JSPROP_PERMANENT isn't a simple composition of the effects that the two flags have separately."

It may eliminate most shared-permanent-appearing-as-own cases, but it doesn't eliminate all of them, so some sort of hacking of enumeration must remain.  Or do I misunderstand your proposal?


> You don't have to give it thought to file a bug to cite in a FIXME

I generally try to avoid filing bugs dependent on a review occurring when I haven't gotten a vote of confidence from the reviewer yet. I don't think I'm anomalous in only lazily filing bugs associated with comments added to the source code.


> Filing a FIXME bug instead of taking the time to write a comment complaining
> about something is clearly the right thing. Wasting time writing the
> complaint is pretty much the wrong thing.

I don't think of it as a complaint but as a lament when I don't see (or don't understand?) a feasible alternative.  I really do wish this assertion convinced me I was wrong to act as I have (although note mistakenness in the last paragraph of this comment); it would make things simpler for all of us if we both thought more similarly (to a degree at least, diversity of thought being a valuable thing).


> No, this would go in existing the last-object-on-proto-chain case code.

Did you mean the |if (JS_UNLIKELY(flags & JSITER_OWNONLY))| case?  I don't see an existing-with-the-patch last-on-proto-chain case that would only address this one esoteric method of access.


> Why are you prematurely optimizing getOwnPropertyNames?

This code in general is shared between for-in and all-properties enumeration, so I believe I am rightly cautious about adding too many digressions for rare cases.  (But see below.)


> It beats hacking on aliases to hide __proto__, especially while at the same
> time complaining about, if not actually working on getting rid of, aliases.
> Why decry one thing while building on it?

If editing the own-only case is what you meant, as in this version of the patch, this modification does indeed seem better.  Please do consider the possibility that I had not considered this alternative to "decrying and building", rather than trying to be intentionally obtuse or plaintive.

Comment 26

[Brendan Eich [:brendan]]

> Please do consider the possibility that I had not considered this alternative
> to "decrying and building", rather than trying to be intentionally obtuse or
> plaintive.

I asked first about the alternative in comment 19, last para. The details of OWNONLY vs. last-proto were not mentioned there. You replied by arguing against the idea in general. The particulars don't have to be worked out to agree that it would be better than building on aliases.

This was the bone of contention. Directional goodness, not every last detail.

Sorry for misremembering the last-proto case -- or imagining it. I saw it in a recent bug, I swear!

/be

Comment 27

[Brendan Eich [:brendan]]

(In reply to comment #26)
> Sorry for misremembering the last-proto case -- or imagining it. I saw it in a
> recent bug, I swear!

Here, I saw it in this bug, in your patch (and in gal's unpatched code):

>+    /*
>+     * It's not necessary to add properties to the hash table at the end of the
>+     * prototype chain.
>+     */
>+    if (pobj->getProto() && !ht.add(p, id))
>+        return false;
>+
>+    if (JS_UNLIKELY(flags & JSITER_OWNONLY)) {
>+        /*
>+         * Per the shared-permanent hack (see js_HasOwnProperty), a property on
>+         * pobj where pobj != obj *might* be a fake-own property if it's
>+         * shared-permanent, pobj and obj have the same class, and it's the
>+         * first property for the id along the prototype chain (shadowing
>+         * implicit in the lookup algorithm).  But eliminate the
>+         * shared-permanent __proto__ found on Object.prototype, because having
>+         * it show up in every object's own properties would result in
>+         * developers unthinkingly chopping off the last element of
>+         * property-name arrays, which would pose problems when we remove
>+         * __proto__ in favor of the ECMAScript-sanctioned alternatives.
>+         *
>+         * FIXME: The shared-permanent hack makes own-property-only enumeration
>+         *        O(protoChainLength * propsPerProto), rather than simply
>+         *        O(propsOnObject) as one would expect.  In the short run we
>+         *        find it expedient to not spend the time to eliminate the
>+         *        hack; in the (hopefully sufficiently) long run this quadratic
>+         *        behavior will come back to bite us.
>+         */
>+        if (!pobj->getProto() && id == ATOM_TO_JSID(cx->runtime->atomState.protoAtom))
>             return true;

Note the existing pobj->getProto() test. I was suggesting adding onto that, or at least capturing itts result. You retest later, which may be ok for all I know.

So again, the last-proto-only case to hang the protoAtom test's hat on was there in your patch and in the unpatched code.

See comment 24 for why you might want to insist on a getter match too.

/be

Comment 28

[Jason Orendorff [:jorendorff]]

Filed:

Bug 575997 - Remove the shared permanent hack (JSPROP_SHARED | JSPROP_PERMANENT)

Bug 576034 - Remove aliased properties (JS_AliasProperty, JS_AliasElement, etc.)

Comment 29

[Jason Orendorff [:jorendorff]]

Comment on attachment 455003 [details] [diff] [review]
Patch, v1.1


In fun_enumerate and regexp_enumerate, consider saving a few lines of
code by using JS_LookupProperty{,ById} instead of js_LookupProperty.

Please return plain `true` and `false` there and in
obj_getOwnPropertyNames.

In jsiter.cpp, Enumerate:
>+         * Per the shared-permanent hack (see js_HasOwnProperty), a property on
>+         * pobj where pobj != obj *might* be a fake-own property if it's
>+         * shared-permanent, pobj and obj have the same class, and it's the
>+         * first property for the id along the prototype chain (shadowing
>+         * implicit in the lookup algorithm).  But eliminate the
>+         * shared-permanent __proto__ found on Object.prototype, because having
>+         * it show up in every object's own properties would result in
>+         * developers unthinkingly chopping off the last element of
>+         * property-name arrays, which would pose problems when we remove
>+         * __proto__ in favor of the ECMAScript-sanctioned alternatives.
>+         *
>+         * FIXME: The shared-permanent hack makes own-property-only enumeration
>+         *        O(protoChainLength * propsPerProto), rather than simply
>+         *        O(propsOnObject) as one would expect.  In the short run we
>+         *        find it expedient to not spend the time to eliminate the
>+         *        hack; in the (hopefully sufficiently) long run this quadratic
>+         *        behavior will come back to bite us.

That's awfully long. How about:

      * Shared-permanent hack: If this property is shared permanent
      * and pobj and obj have the same class, then treat it as an own
      * property of obj, even if pobj != obj. (But see bug 575997.)
      *
      * Omit the magic __proto__ property so that JS code can use
      * Object.getOwnPropertyNames without worrying about it.

Please leave a comment in bug 575997 about the performance cost here
instead of a source code comment.

In jsobj.cpp, js_HasOwnProperty:
>              * without the ability to remove and recreate (with differences)
>              * the property, there is no way to tell whether it is directly
>              * owned, or indirectly delegated.
>+             *
>+             * This hack is also propagated through the Enumerate method in
>+             * jsiter.cpp.
>              */

I don't think this addition is worth 3 lines; `grep -i sharedpermanent`
works for me.

In obj_getOwnPropertyNames:
>+    if (argc == 0) {
>+        JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_MORE_ARGS_NEEDED,
>+                             "Object.getOwnPropertyNames", "0", "s");
>+        return JS_FALSE;
>+    }
>+
>+    jsval v = vp[2];
>+    if (JSVAL_IS_PRIMITIVE(v)) {
>+        char *bytes = js_DecompileValueGenerator(cx, JSDVG_SEARCH_STACK, v, NULL);
>+        if (!bytes)
>+            return JS_FALSE;
>+        JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_UNEXPECTED_TYPE,
>+                             bytes, "not an object");
>+        JS_free(cx, bytes);
>+        return JS_FALSE;
>+    }

There's very similar code in obj_defineProperty and elsewhere. Factor it out?

>+    jsint len = props.length();
>+    JSObject *aobj = js_NewArrayObject(cx, len, const_cast<jsval *>(props.begin()));

This const_cast isn't necessary. The function takes a const jsval *.
After that the temporary `len` can go away.

In jstypedarray.cpp:
>+UINT_AND_BIT_TO_JSVAL(uint32 j, bool b)
>+JSVAL_TO_UINT_AND_BIT(jsval v, uint32 &j, bool &b)
[...]

Consider instead using JSVAL_TRUE or INT_TO_JSVAL(-1) as the special
*statep value. I claim you don't really need a pair (uint, bool); a
single special value should work fine.

In dom/base/nsDOMClassInfo.cpp, nsWindowSH::NewEnumerate:
>   switch ((JSIterateOp)enum_op) {
>     case JSENUMERATE_INIT:
>+    case JSENUMERATE_INIT_ALL: /* FIXME: non-enumerable property support */
>     {

Please file the follow-up bug and cite it in this comment.

In js/src/tests/ecma_5/Object/15.2.3.4-03.js:
>+function arraysEqual(a1, a2)
>+{
>+  return a1.length === a2.length &&
>+         a1.every(function(v, i) { return v === a2[i]; });
>+}
>+
>+
>+assertEq(Object.getOwnPropertyNames(arraysEqual).indexOf("length") >= 0, true);

It seems like you don't actually need arraysEqual in this one; any
function would do.

r=me with those changes!

Comment 30

[Robert Sayre]

http://hg.mozilla.org/tracemonkey/rev/f6e0fbe936bd

woohoo!

Comment 31

[Jeff Walden [:Waldo]]

(In reply to comment #29)
> In fun_enumerate and regexp_enumerate, consider saving a few lines of
> code by using JS_LookupProperty{,ById} instead of js_LookupProperty.

Well called.


> There's very similar code in obj_defineProperty and elsewhere. Factor it out?

Done, to a mess of other methods as well.


> This const_cast isn't necessary.

Right; this wasn't the case at some time in the past, or amidst method-name renames the constness of the method changed or something and I didn't notice.


> Consider instead using JSVAL_TRUE or INT_TO_JSVAL(-1) as the special
> *statep value. I claim you don't really need a pair (uint, bool); a
> single special value should work fine.

Quite true.  I did need them when I was meticulously making "length" the last property to be enumerated, but then I changed that to be first for parity with dense arrays, and I didn't consider that the change meant the bit-packing scheme was totally (not just mostly) superfluous.


> In dom/base/nsDOMClassInfo.cpp, nsWindowSH::NewEnumerate:
> >   switch ((JSIterateOp)enum_op) {
> >     case JSENUMERATE_INIT:
> >+    case JSENUMERATE_INIT_ALL: /* FIXME: non-enumerable property support */
> >     {
> 
> Please file the follow-up bug and cite it in this comment.

Bug 576449 now.

> In js/src/tests/ecma_5/Object/15.2.3.4-03.js:
> >+function arraysEqual(a1, a2)
> >+{
> >+  return a1.length === a2.length &&
> >+         a1.every(function(v, i) { return v === a2[i]; });
> >+}
> >+
> >+
> >+assertEq(Object.getOwnPropertyNames(arraysEqual).indexOf("length") >= 0, true);
> 
> It seems like you don't actually need arraysEqual in this one; any
> function would do.

True.  I actually expanded this test a bit to cover more instances than just this one, and I snuck in some feature-testing of bound functions pre-their introduction to SpiderMonkey since Function.prototype.bind is on my mind now and bound functions have semi-weird lengths.

Comment 32

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/f6e0fbe936bd

Comment 33

[Fabian (Crash) Grutschus]

dev-doc under:
https://developer.mozilla.org/en/Core_JavaScript_1.5_Reference/Global_Objects/Object/getOwnPropertyNames