Closed
Bug 308856
Opened 19 years ago
Closed 19 years ago
Global js variables don't show up when enumerating [Window]
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: boogs, Assigned: mrbkap)
References
Details
(Keywords: fixed1.8, Whiteboard: [needs branch checkin])
Attachments
(4 files, 1 obsolete file)
(deleted),
text/html
|
Details | |
(deleted),
patch
|
mrbkap
:
review+
shaver
:
superreview+
asa
:
approval1.8b5+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
brendan
:
review+
jst
:
superreview+
asa
:
approval1.8b5+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
brendan
:
review+
asa
:
approval1.8b5+
|
Details | Diff | Splinter Review |
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•19 years ago
|
||
Comment 2•19 years ago
|
||
Must fix for 1.5.
/be
Flags: blocking1.8b5+
OS: Windows XP → All
Hardware: PC → All
Updated•19 years ago
|
Blocks: splitwindows
Assignee | ||
Comment 3•19 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
Comment 4•19 years ago
|
||
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 5•19 years ago
|
||
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)
Comment 6•19 years ago
|
||
Attachment #197009 -
Flags: superreview?(shaver)
Attachment #197009 -
Flags: review?(mrbkap)
Updated•19 years ago
|
Whiteboard: [needs review mrbkap, SR shaver]
Assignee | ||
Comment 7•19 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•19 years ago
|
||
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•19 years ago
|
||
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•19 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•19 years ago
|
Status: NEW → ASSIGNED
Comment 11•19 years ago
|
||
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•19 years ago
|
||
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 13•19 years ago
|
||
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•19 years ago
|
Attachment #197490 -
Flags: superreview?(jst)
Assignee | ||
Comment 15•19 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 16•19 years ago
|
||
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•19 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
Closed: 19 years ago
Resolution: --- → FIXED
Whiteboard: [needs review mrbkap, SR shaver] → [needs branch checkin]
Assignee | ||
Updated•19 years ago
|
Attachment #197494 -
Flags: superreview?(shaver) → approval1.8b5?
Assignee | ||
Comment 18•19 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•19 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•19 years ago
|
Attachment #197009 -
Flags: approval1.8b5? → approval1.8b5+
Updated•19 years ago
|
Attachment #197490 -
Flags: approval1.8b5? → approval1.8b5+
Updated•19 years ago
|
Attachment #197494 -
Flags: approval1.8b5? → approval1.8b5+
Comment 20•19 years ago
|
||
please get this landed today. thanks.
Comment 22•18 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+
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•