Last Comment Bug 606573 - [i for (i in Iterator(object from another scope))] -> []
: [i for (i in Iterator(object from another scope))] -> []
Status: RESOLVED FIXED
[compartments], fixed-in-tracemonkey
: regression
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: ---
Assigned To: Andreas Gal :gal
:
Mentors:
Depends on: 664250
Blocks: 607113 607131
  Show dependency treegraph
 
Reported: 2010-10-22 14:09 PDT by Ed Lee :Mardak
Modified: 2014-06-14 11:07 PDT (History)
8 users (show)
brendan: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
beta7+


Attachments
patch (4.00 KB, patch)
2010-10-22 19:12 PDT, Andreas Gal :gal
mrbkap: review+
Details | Diff | Review
patch (6.81 KB, patch)
2010-10-23 16:16 PDT, Andreas Gal :gal
mrbkap: review+
Details | Diff | Review

Description Ed Lee :Mardak 2010-10-22 14:09:02 PDT
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]
Comment 1 Ed Lee :Mardak 2010-10-22 14:21:58 PDT
Fail minefield 20101014 http://hg.mozilla.org/mozilla-central/rev/ad0a0be8be74
Pass minefield 20101013 http://hg.mozilla.org/mozilla-central/rev/178f26e21cfc
Comment 2 Andreas Gal :gal 2010-10-22 19:11:34 PDT
Should get fixed for final.
Comment 3 Andreas Gal :gal 2010-10-22 19:12:00 PDT
Created attachment 485461 [details] [diff] [review]
patch
Comment 4 Andreas Gal :gal 2010-10-22 19:13:07 PDT
Edward, want to apply this and give it some testing?
Comment 5 Blake Kaplan (:mrbkap) (please use needinfo!) 2010-10-22 19:45:40 PDT
Comment on attachment 485461 [details] [diff] [review]
patch

>+        /* Translate StopIteration singleton. */

Nit: add "the" before StopIteration.
Comment 6 Ed Lee :Mardak 2010-10-23 14:19:11 PDT
(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.
Comment 7 Andreas Gal :gal 2010-10-23 16:16:40 PDT
Created attachment 485545 [details] [diff] [review]
patch

Also freeze StopIteration and StopIteration.prototype
Comment 8 Andreas Gal :gal 2010-10-23 16:25:59 PDT
http://hg.mozilla.org/tracemonkey/rev/0bba47cbd1de
Comment 9 Brendan Eich [:brendan] 2010-10-23 17:00:37 PDT
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 Brendan Eich [:brendan] 2010-10-23 17:03:14 PDT
(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
Comment 11 Andreas Gal :gal 2010-10-23 22:21:01 PDT
Yeah, sounds reasonable. I will fix in a separate bug.
Comment 12 Andreas Gal :gal 2010-10-24 00:58:38 PDT
Follow-up http://hg.mozilla.org/tracemonkey/rev/c0b8cfea75ba

I am not attached to the flags, happy to remove them.
Comment 13 Jonas Sicking (:sicking) PTO Until July 5th 2010-10-25 16:41:07 PDT
They made me do it judge, I swear!
Comment 14 Damon Sicore (:damons) 2010-10-26 10:32:23 PDT
Comment 2 says this should be fixed for final.  Why is this blocking beta 7?
Comment 15 Peter Van der Beken [:peterv] 2010-10-26 10:33:03 PDT
Because it's needed to fix Jetpack.
Comment 16 Brendan Eich [:brendan] 2010-10-26 12:10:12 PDT
No testcase, bad bad icky bad. Please fix ASAP.

/be
Comment 18 Robert Sayre 2010-10-27 06:31:29 PDT
Seriously, let's get a test in here.
Comment 19 Andreas Gal :gal 2011-06-13 23:27:50 PDT
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 Brendan Eich [:brendan] 2011-06-14 00:11:18 PDT
That's "in which case we *can* get rid of..." right?

Followup bug, if you please.

/be
Comment 21 Brendan Eich [:brendan] 2011-06-14 12:01:38 PDT
Filed: bug 664250 to remove the JSCLASS flags and one-off freeze in jsiter.cpp.

/be

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