Last Comment Bug 308856 - Global js variables don't show up when enumerating [Window]
: Global js variables don't show up when enumerating [Window]
Status: RESOLVED FIXED
[needs branch checkin]
: fixed1.8
Product: Core
Classification: Components
Component: DOM (show other bugs)
: 1.8 Branch
: All All
: -- normal (vote)
: ---
Assigned To: Blake Kaplan (:mrbkap)
: Hixie (not reading bugmail)
Mentors:
Depends on: 310742
Blocks: splitwindows 310864
  Show dependency treegraph
 
Reported: 2005-09-16 12:36 PDT by Aaron Boodman
Modified: 2006-10-29 10:44 PST (History)
9 users (show)
brendan: blocking1.8b5+
sayrer: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (167 bytes, text/html)
2005-09-16 12:36 PDT, Aaron Boodman
no flags Details
proposed JS API additions to help mrbkap avoid recursive death (6.44 KB, patch)
2005-09-21 22:35 PDT, Brendan Eich [:brendan]
no flags Details | Diff | Splinter Review
that's more like it (7.32 KB, patch)
2005-09-21 22:53 PDT, Brendan Eich [:brendan]
mrbkap: review+
shaver: superreview+
asa: approval1.8b5+
Details | Diff | Splinter Review
working patch (6.83 KB, patch)
2005-09-26 16:56 PDT, Blake Kaplan (:mrbkap)
brendan: review+
jst: superreview+
asa: approval1.8b5+
Details | Diff | Splinter Review
Easy jseng fix (2.10 KB, patch)
2005-09-26 17:54 PDT, Blake Kaplan (:mrbkap)
brendan: review+
asa: approval1.8b5+
Details | Diff | Splinter Review

Description Aaron Boodman 2005-09-16 12:36:04 PDT
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 2005-09-16 12:36:33 PDT
Created attachment 196351 [details]
testcase
Comment 2 Brendan Eich [:brendan] 2005-09-16 12:45:31 PDT
Must fix for 1.5.

/be
Comment 3 Blake Kaplan (:mrbkap) 2005-09-16 15:52:12 PDT
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] 2005-09-21 22:35:16 PDT
Created attachment 197006 [details] [diff] [review]
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] 2005-09-21 22:38:57 PDT
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] 2005-09-21 22:53:01 PDT
Created attachment 197009 [details] [diff] [review]
that's more like it
Comment 7 Blake Kaplan (:mrbkap) 2005-09-24 01:24:15 PDT
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) 2005-09-26 16:56:23 PDT
Created attachment 197490 [details] [diff] [review]
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) 2005-09-26 17:54:47 PDT
Created attachment 197494 [details] [diff] [review]
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) 2005-09-26 17:58:19 PDT
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] 2005-09-29 15:06:48 PDT
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] 2005-09-29 15:09:51 PDT
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] 2005-09-29 15:48:59 PDT
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) 2005-09-29 16:05:25 PDT
Comment on attachment 197009 [details] [diff] [review]
that's more like it

sr=shaver, sorry for the delay.
Comment 15 Blake Kaplan (:mrbkap) 2005-09-29 16:41:01 PDT
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, jst@mozilla.com) 2005-09-29 17:26:12 PDT
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) 2005-09-29 18:19:54 PDT
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) 2005-09-29 18:29:09 PDT
Comment on attachment 197009 [details] [diff] [review]
that's more like it

This is an api addition - no risk.
Comment 19 Blake Kaplan (:mrbkap) 2005-09-29 18:29:53 PDT
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] 2005-09-30 12:07:42 PDT
please get this landed today. thanks.
Comment 21 Blake Kaplan (:mrbkap) 2005-09-30 14:21:14 PDT
Fixes checked into MOZILLA_1_8_BRANCH.
Comment 22 Robert Sayre 2006-10-29 10:44:03 PST
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

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