606573 - [i for (i in Iterator(object from another scope))] -> []

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Ed Lee :Mardak]

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]

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]

Should get fixed for final.

Comment 3

[Andreas Gal :gal]

Attachment: patch

Comment 4

[Andreas Gal :gal]

Edward, want to apply this and give it some testing?

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 485461 [details] [diff] [review]
patch

>+        /* Translate StopIteration singleton. */

Nit: add "the" before StopIteration.

Comment 6

[Ed Lee :Mardak]

(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/[e-mail]-73f6db14a450/tryserver-macosx64/

This particular issue is fixed for me with that build.

Comment 7

[Andreas Gal :gal]

Attachment: patch

Also freeze StopIteration and StopIteration.prototype

Comment 8

[Andreas Gal :gal]

http://hg.mozilla.org/tracemonkey/rev/0bba47cbd1de

Comment 9

[Brendan Eich [:brendan]]

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]]

(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]

Yeah, sounds reasonable. I will fix in a separate bug.

Comment 12

[Andreas Gal :gal]

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) No longer reading bugmail consistently]

They made me do it judge, I swear!

Comment 14

[Damon Sicore (:damons)]

Comment 2 says this should be fixed for final.  Why is this blocking beta 7?

Comment 15

[Peter Van der Beken [:peterv]]

Because it's needed to fix Jetpack.

Comment 16

[Brendan Eich [:brendan]]

No testcase, bad bad icky bad. Please fix ASAP.

/be

Comment 17

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/0bba47cbd1de

Comment 18

[Robert Sayre]

Seriously, let's get a test in here.

Comment 19

[Andreas Gal :gal]

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]]

That's "in which case we *can* get rid of..." right?

Followup bug, if you please.

/be

Comment 21

[Brendan Eich [:brendan]]

Filed: bug 664250 to remove the JSCLASS flags and one-off freeze in jsiter.cpp.

/be