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

RESOLVED FIXED

Status

()

Core
DOM
RESOLVED FIXED
12 years ago
11 years ago

People

(Reporter: Aaron Boodman, Assigned: mrbkap)

Tracking

({fixed1.8})

1.8 Branch
fixed1.8
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8b5 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [needs branch checkin])

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

12 years ago
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.
(Reporter)

Comment 1

12 years ago
Created attachment 196351 [details]
testcase
Must fix for 1.5.

/be
Flags: blocking1.8b5+
OS: Windows XP → All
Hardware: PC → All

Updated

12 years ago
Blocks: 296639
(Assignee)

Comment 3

12 years ago
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.
Assignee: general → mrbkap
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
Attachment #197006 - Flags: superreview?(shaver)
Attachment #197006 - Flags: review?(mrbkap)
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
Attachment #197006 - Attachment is obsolete: true
Attachment #197006 - Flags: superreview?(shaver)
Attachment #197006 - Flags: review?(mrbkap)
Created attachment 197009 [details] [diff] [review]
that's more like it
Attachment #197009 - Flags: superreview?(shaver)
Attachment #197009 - Flags: review?(mrbkap)

Updated

12 years ago
Whiteboard: [needs review mrbkap, SR shaver]
(Assignee)

Comment 7

12 years ago
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!
Attachment #197009 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 8

12 years ago
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?
(Assignee)

Comment 9

12 years ago
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.
Attachment #197494 - Flags: superreview?(shaver)
Attachment #197494 - Flags: review?(brendan)
(Assignee)

Comment 10

12 years ago
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.
(Assignee)

Updated

12 years ago
Status: NEW → ASSIGNED
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 on attachment 197490 [details] [diff] [review]
working patch

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

/be
Attachment #197490 - Flags: review+
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
Attachment #197494 - Flags: review?(brendan) → review+
Comment on attachment 197009 [details] [diff] [review]
that's more like it

sr=shaver, sorry for the delay.
Attachment #197009 - Flags: superreview?(shaver) → superreview+
(Assignee)

Updated

12 years ago
Attachment #197490 - Flags: superreview?(jst)
(Assignee)

Comment 15

12 years ago
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 on attachment 197490 [details] [diff] [review]
working patch

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

sr=jst
Attachment #197490 - Attachment description: non-working patch → working patch
Attachment #197490 - Flags: superreview?(jst) → superreview+
(Assignee)

Comment 17

12 years ago
And attachment 197490 [details] [diff] [review] has been checked into the trunk. I'll check it into the
branch tomorrow.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Whiteboard: [needs review mrbkap, SR shaver] → [needs branch checkin]
(Assignee)

Updated

12 years ago
Attachment #197494 - Flags: superreview?(shaver) → approval1.8b5?
(Assignee)

Comment 18

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

This is an api addition - no risk.
Attachment #197009 - Flags: approval1.8b5?
(Assignee)

Comment 19

12 years ago
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.
Attachment #197490 - Flags: approval1.8b5?

Updated

12 years ago
Attachment #197009 - Flags: approval1.8b5? → approval1.8b5+

Updated

12 years ago
Attachment #197490 - Flags: approval1.8b5? → approval1.8b5+

Updated

12 years ago
Attachment #197494 - Flags: approval1.8b5? → approval1.8b5+

Comment 20

12 years ago
please get this landed today. thanks.
(Assignee)

Comment 21

12 years ago
Fixes checked into MOZILLA_1_8_BRANCH.
Keywords: fixed1.8
(Assignee)

Updated

12 years ago
Depends on: 310742
Blocks: 310864

Comment 22

11 years ago
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
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.