308856 - Global js variables don't show up when enumerating [Window]

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Aaron Boodman]

Since [Window] is the global object variables created via var x=y should show up
when enumerating with |for in|. This worked in previous FFes but now is broken.

Comment 1

[Aaron Boodman]

Attachment: testcase

Comment 2

[Brendan Eich [:brendan]]

Must fix for 1.5.

/be

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

I'll take a whack at this. It looks like JSCLASS_NEW_ENUMERATE is going to make
a grand re-entrance into the Mozilla source base.

Comment 4

[Brendan Eich [:brendan]]

Attachment: proposed JS API additions to help mrbkap avoid recursive death

This is the minimum I can justify.  See the XXX comment in jsapi.c.

/be

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 197006 [details] [diff] [review]
proposed JS API additions to help mrbkap avoid recursive death

Oops, forgot to insist on JSPROP_ENUMERATE and deal with middle-delete and
other hard cases for native properties!

/be

Comment 6

[Brendan Eich [:brendan]]

Attachment: that's more like it

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 197009 [details] [diff] [review]
that's more like it

It sure is useful to have all of those extra slots lying around ;-).

r=mrbkap, thanks!

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: working patch

The JS engine makes this otherwise fine patch [;-)] not work. It iterates over
the properties just fine, but the engine refuses to acknowledge the returned
properties because they're not on the object being enumerated (see the |!prop
|| obj != obj2| check in jsinterp.c).

Unfortunately, this seems to be a showstopper for this patch. More JS api fixes
are needed.

There are a few options that would fix this bug:
* Add yet another JS enumerate api (vetoed by everybody involved, I think).
* Instead of the simplistic obj != obj2 check in jsinterp.c, actually walk the
proto chain (hopefully shallow!) to see if obj2 is, in fact, in obj's proto
chain, and if it is, ignore the property.
* Instead of the simplistic obj != obj2 check, add found properties to a hash
table (or a vector/hash table combination, whatever) and not do the check for
the objects being the same, simply ask if we've already enumerated them (I
don't think that we ever want to enumerate the same object twice, though that
would change with this approach).

Brendan, do you have any thoughts here?

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Easy jseng fix

Brendan, is this too offensive? jst points out (probably rightly) that proto
chains are usually not deeper than 3 or 4 protos deep, so the loop is probably
not terribly expensive.

My patch does, in fact, work with this patch applied.

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 197494 [details] [diff] [review]
Easy jseng fix

Please imagine that I didn't mix up the names of the parameters of
ObjectInProtoChain.

Comment 11

[Brendan Eich [:brendan]]

Comment on attachment 197490 [details] [diff] [review]
working patch

>+      // Great, we have the js object, now lets enumerate it.

let's

>+      JSObject *iterator = JS_NewPropertyIterator(cx, enumobj);
>+      if (!iterator) {
>+        return NS_ERROR_OUT_OF_MEMORY;
>+      }
>+
>+      *statep = PRIVATE_TO_JSVAL(iterator);

OBJECT_TO_JSVAL, not PRIVATE -- or the GC won't mark this slot and you'll have
bugs about dead iterators back from the grave, eating the brains of this code
when it calls back into them.

>+    case JSENUMERATE_NEXT:
>+    {
>+      JSObject *iterator = (JSObject*)JSVAL_TO_PRIVATE(*statep);

JSVAL_TO_OBJECT here, etc.

/be

Comment 12

[Brendan Eich [:brendan]]

Comment on attachment 197490 [details] [diff] [review]
working patch

r=me with the private tagging fixed to use object (noop) tagging.

/be

Comment 13

[Brendan Eich [:brendan]]

Comment on attachment 197494 [details] [diff] [review]
Easy jseng fix

> JSBool
>+ObjectInProtoChain(JSContext *cx, JSObject *obj, JSObject *maybeproto)
>+{
>+    JS_ASSERT(maybeproto);
>+
>+    /* Note: This loop purposely skips checking obj != maybeproto. */
>+    do {
>+        maybeproto = JSVAL_TO_OBJECT(OBJ_GET_SLOT(cx, maybeproto, JSSLOT_PROTO));

Use OBJ_GET_PROTO, eh?

>             /* If the id was deleted, or found in a prototype, skip it. */
>-            if (!prop || obj2 != obj)
>+            if (!prop || (obj != obj2 && ObjectInProtoChain(cx, obj2, obj)))

obj, obj2 as you noted.

r=me with those fixes.

/be

Comment 14

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 197009 [details] [diff] [review]
that's more like it

sr=shaver, sorry for the delay.

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

Attachment 197494 [details] [diff] and attachment 197009 [details] [diff] [review] have been checked into trunk. This isn't
quite fixed yet (though the js engine is ready!).

Comment 16

[Johnny Stenback (:jst)]

Comment on attachment 197490 [details] [diff] [review]
working patch

Make sure all your debug printf's are in #ifdef ... blocks.

sr=jst

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

And attachment 197490 [details] [diff] [review] has been checked into the trunk. I'll check it into the
branch tomorrow.

Comment 18

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 197009 [details] [diff] [review]
that's more like it

This is an api addition - no risk.

Comment 19

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 197490 [details] [diff] [review]
working patch

This is a low-risk patch that allows people to use for-in style loops over the
|window| object. It should only affect those uses.

Comment 20

[Asa Dotzler [:asa]]

please get this landed today. thanks.

Comment 21

[Blake Kaplan (:mrbkap) (inactive)]

Fixes checked into MOZILLA_1_8_BRANCH.

Comment 22

[Robert Sayre]

Added to mochitest

RCS file: /cvsroot/mozilla/testing/mochitest/tests/test_bug308856.html,v
done
Checking in tests/test_bug308856.html;
/cvsroot/mozilla/testing/mochitest/tests/test_bug308856.html,v  <--  test_bug308856.html
initial revision: 1.1
done