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

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
7 years ago
3 years ago

People

(Reporter: Mardak, Assigned: gal)

Tracking

({regression})

Trunk
x86
Mac OS X
regression
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(blocking2.0 beta7+)

Details

(Whiteboard: [compartments], fixed-in-tracemonkey)

Attachments

(1 attachment, 1 obsolete attachment)

6.81 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
(Reporter)

Description

7 years ago
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

7 years ago
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

Updated

7 years ago
Assignee: general → gal
(Assignee)

Comment 2

7 years ago
Should get fixed for final.
(Assignee)

Comment 3

7 years ago
Created attachment 485461 [details] [diff] [review]
patch
(Assignee)

Updated

7 years ago
Attachment #485461 - Flags: review?(mrbkap)
(Assignee)

Comment 4

7 years ago
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+
(Reporter)

Comment 6

7 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

7 years ago
Created attachment 485545 [details] [diff] [review]
patch

Also freeze StopIteration and StopIteration.prototype
(Assignee)

Updated

7 years ago
Attachment #485545 - Attachment is patch: true
Attachment #485545 - Attachment mime type: application/octet-stream → text/plain
Attachment #485545 - Flags: review?(mrbkap)

Updated

7 years ago
Attachment #485545 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 8

7 years ago
http://hg.mozilla.org/tracemonkey/rev/0bba47cbd1de
(Assignee)

Updated

7 years ago
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
(Assignee)

Comment 11

7 years ago
Yeah, sounds reasonable. I will fix in a separate bug.
(Assignee)

Comment 12

7 years ago
Follow-up http://hg.mozilla.org/tracemonkey/rev/c0b8cfea75ba

I am not attached to the flags, happy to remove them.
(Reporter)

Updated

7 years ago
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?
Blocks: 607131

Comment 17

7 years ago
http://hg.mozilla.org/mozilla-central/rev/0bba47cbd1de
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED

Comment 18

7 years ago
Seriously, let's get a test in here.
(Assignee)

Comment 19

6 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.
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.