Closed
Bug 606573
Opened 14 years ago
Closed 14 years ago
[i for (i in Iterator(object from another scope))] -> []
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
blocking2.0 | --- | beta7+ |
People
(Reporter: Mardak, Assigned: gal)
References
Details
(Keywords: regression, Whiteboard: [compartments], fixed-in-tracemonkey)
Attachments
(1 file, 1 obsolete file)
6.81 KB,
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
I ran into this while investigating bug 604641, but this might end up being more like bug 604523.
From a module, I have
function foo() {
return { Cc: Components.classes, Ci: Components.interfaces, .. };
}
From a restartless addon, I import the module and call the function:
let x = foo();
Cu.reportError(Object.keys(x));
EXPECT: Cc,Ci,Cu,Cr,Cm,components
ACTUAL: Cc,Ci,Cu,Cr,Cm,components
let it = Iterator(x);
Cu.reportError(it.next());
EXPECT: Cc,[object nsXPCComponents_Classes]
ACTUAL: Cc,[object nsXPCComponents_Classes]
Cu.reportError([i for (i in it)]);
EXPECT: Ci,[object nsXPCComponents_Interfaces],Cu,[object nsXPCComponents_Utils]..
ACTUAL:
Cu.reportError(it.next());
EXPECT: Exception calling callback: [object StopIteration]
ACTUAL: Ci,[object nsXPCComponents_Interfaces]
Reporter | ||
Comment 1•14 years ago
|
||
Fail minefield 20101014 http://hg.mozilla.org/mozilla-central/rev/ad0a0be8be74
Pass minefield 20101013 http://hg.mozilla.org/mozilla-central/rev/178f26e21cfc
Updated•14 years ago
|
Assignee: general → gal
Assignee | ||
Comment 2•14 years ago
|
||
Should get fixed for final.
Assignee | ||
Comment 3•14 years ago
|
||
Assignee | ||
Updated•14 years ago
|
Attachment #485461 -
Flags: review?(mrbkap)
Assignee | ||
Comment 4•14 years ago
|
||
Edward, want to apply this and give it some testing?
Comment 5•14 years ago
|
||
Comment on attachment 485461 [details] [diff] [review]
patch
>+ /* Translate StopIteration singleton. */
Nit: add "the" before StopIteration.
Attachment #485461 -
Flags: review?(mrbkap) → review+
Reporter | ||
Comment 6•14 years ago
|
||
(In reply to comment #4)
> Edward, want to apply this and give it some testing?
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/edward.lee@engineering.uiuc.edu-73f6db14a450/tryserver-macosx64/
This particular issue is fixed for me with that build.
Assignee | ||
Comment 7•14 years ago
|
||
Also freeze StopIteration and StopIteration.prototype
Assignee | ||
Updated•14 years ago
|
Attachment #485545 -
Attachment is patch: true
Attachment #485545 -
Attachment mime type: application/octet-stream → text/plain
Attachment #485545 -
Flags: review?(mrbkap)
Updated•14 years ago
|
Attachment #485545 -
Flags: review?(mrbkap) → review+
Assignee | ||
Comment 8•14 years ago
|
||
Assignee | ||
Updated•14 years ago
|
Whiteboard: [compartments] → [compartments], fixed-in-tracemonkey
Comment 9•14 years ago
|
||
Nit: space around binary operators in
4.7 + if (clasp->flags & (JSCLASS_FREEZE_PROTO|JSCLASS_FREEZE_CTOR)) {
Non-nit: running out of JSClass.flags bits, I would have done this one ad-hoc in jsiter.cpp instead of claiming those two.
Not sure we can use them elsewhere given backward compat, and I'm pretty sure we should not: JS is monkey-patchable and this is a net win; it is not going away, although there may be some built-in classes or objects that are frozen for good reason (module reflections).
http://wiki.ecmascript.org/doku.php?id=strawman:iterators#stopiteration does not specify frozen StopIteration.
Non-nit: there is no StopIteration.prototype, it is a class akin to Math that is initialized (via js_InitClass) with a null constructor parameter. This means at the point you test these flags, ctor == proto and you are going to freeze the same object twice. So at least lose JSCLASS_FREEZE_CTOR since StopIteration is not a ctor.
rs=me on followup to do that minimal fix, but I think we should reconsider the JSCLASS_ flag consumption.
/be
Comment 10•14 years ago
|
||
(In reply to comment #9)
> at the point you test these flags, ctor == proto and you are going to freeze
> the same object twice. So at least lose JSCLASS_FREEZE_CTOR since StopIteration
> is not a ctor.
>
> rs=me on followup to do that minimal fix, but I think we should reconsider the
> JSCLASS_ flag consumption.
I think that minimal fix should not just remove JSCLASS_FREEZE_CTOR from StopIteration's class flags -- it should also avoid double freezing by testing ctor != proto in the js_InitClass code you added, and adding something like JS_ASSERT_IF(ctor == proto, !(clasp->flags & JSCLASS_FREEZE_CTOR)) to boot.
/be
Assignee | ||
Comment 11•14 years ago
|
||
Yeah, sounds reasonable. I will fix in a separate bug.
Assignee | ||
Comment 12•14 years ago
|
||
Follow-up http://hg.mozilla.org/tracemonkey/rev/c0b8cfea75ba
I am not attached to the flags, happy to remove them.
Updated•14 years ago
|
Attachment #485461 -
Attachment is obsolete: true
Updated•14 years ago
|
blocking2.0: --- → ?
They made me do it judge, I swear!
blocking2.0: ? → beta7+
Comment 14•14 years ago
|
||
Comment 2 says this should be fixed for final. Why is this blocking beta 7?
Comment 15•14 years ago
|
||
Because it's needed to fix Jetpack.
Comment 17•14 years ago
|
||
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Comment 18•14 years ago
|
||
Seriously, let's get a test in here.
Assignee | ||
Comment 19•14 years ago
|
||
We should improve the API here. Currently for backwards compatibility we have a per-compartment singleton StopIteration object all incoming objects of stop-iteration class are translated to so instance of works as expected. We should have something like Iteration.isStopIteration(obj) instead, in which case we can't get rid of all the FREEZE gunk and the per-compartment singleton.
Comment 20•14 years ago
|
||
That's "in which case we *can* get rid of..." right?
Followup bug, if you please.
/be
Comment 21•14 years ago
|
||
Filed: bug 664250 to remove the JSCLASS flags and one-off freeze in jsiter.cpp.
/be
You need to log in
before you can comment on or make changes to this bug.
Description
•