Closed Bug 606573 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set

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)

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]
Fail minefield 20101014 http://hg.mozilla.org/mozilla-central/rev/ad0a0be8be74
Pass minefield 20101013 http://hg.mozilla.org/mozilla-central/rev/178f26e21cfc
Keywords: regression
Whiteboard: [compartments]
Version: unspecified → Trunk
Assignee: general → gal
Should get fixed for final.
Attached patch patch (obsolete) — Splinter Review
Attachment #485461 - Flags: review?(mrbkap)
Edward, want to apply this and give it some testing?
Comment on attachment 485461 [details] [diff] [review]
patch

>+        /* Translate StopIteration singleton. */

Nit: add "the" before StopIteration.
Attachment #485461 - Flags: review?(mrbkap) → review+
(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.
Attached patch patchSplinter Review
Also freeze StopIteration and StopIteration.prototype
Attachment #485545 - Attachment is patch: true
Attachment #485545 - Attachment mime type: application/octet-stream → text/plain
Attachment #485545 - Flags: review?(mrbkap)
Attachment #485545 - Flags: review?(mrbkap) → review+
Whiteboard: [compartments] → [compartments], fixed-in-tracemonkey
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
(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
Yeah, sounds reasonable. I will fix in a separate bug.
Follow-up http://hg.mozilla.org/tracemonkey/rev/c0b8cfea75ba

I am not attached to the flags, happy to remove them.
Blocks: 607113
Attachment #485461 - Attachment is obsolete: true
blocking2.0: --- → ?
They made me do it judge, I swear!
blocking2.0: ? → beta7+
Comment 2 says this should be fixed for final.  Why is this blocking beta 7?
Because it's needed to fix Jetpack.
No testcase, bad bad icky bad. Please fix ASAP.

/be
Flags: in-testsuite?
http://hg.mozilla.org/mozilla-central/rev/0bba47cbd1de
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Seriously, let's get a test in here.
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.
That's "in which case we *can* get rid of..." right?

Followup bug, if you please.

/be
Depends on: 664250
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.