Last Comment Bug 349326 - for-in loops should always close iterator object
: for-in loops should always close iterator object
Status: VERIFIED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: mozilla1.9alpha1
Assigned To: Igor Bukanov
:
:
Mentors:
Depends on: 379758 381973 387871
Blocks: geniter js1.8 380469
  Show dependency treegraph
 
Reported: 2006-08-19 10:01 PDT by Igor Bukanov
Modified: 2007-09-18 15:01 PDT (History)
9 users (show)
bob: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
implementation v0.1 (23.21 KB, patch)
2007-05-16 15:47 PDT, Igor Bukanov
no flags Details | Diff | Splinter Review
implementation v0.9 (29.01 KB, patch)
2007-05-25 17:27 PDT, Igor Bukanov
no flags Details | Diff | Splinter Review
test case for generator close (2.40 KB, text/plain)
2007-05-25 17:30 PDT, Igor Bukanov
no flags Details
implementation v1 (53.33 KB, patch)
2007-06-05 11:20 PDT, Igor Bukanov
no flags Details | Diff | Splinter Review
implementation v2 (50.40 KB, patch)
2007-06-14 12:48 PDT, Igor Bukanov
no flags Details | Diff | Splinter Review
implementation v2 (diff -b version) (46.63 KB, patch)
2007-06-14 12:51 PDT, Igor Bukanov
no flags Details | Diff | Splinter Review
diff between v1 and v2 (10.61 KB, text/plain)
2007-06-14 12:53 PDT, Igor Bukanov
no flags Details
implementation v3 (51.22 KB, patch)
2007-06-26 03:36 PDT, Igor Bukanov
brendan: review+
Details | Diff | Splinter Review
diff between v2 and v3 (23.88 KB, text/plain)
2007-06-26 03:40 PDT, Igor Bukanov
no flags Details
implementation v3b (50.69 KB, patch)
2007-07-01 16:32 PDT, Igor Bukanov
igor: review+
Details | Diff | Splinter Review

Description Igor Bukanov 2006-08-19 10:01:41 PDT
Consider the following example for js shell:

var was_closed = false;
function gen() {
        try {
                yield 1;
        } finally {
                was_closed = true;
        }
}

for (var i in gen())
        break;

gc();

print("AFTER FIRST GC: was_closed="+was_closed);

for (var i in (function() { yield 0; })) { }
gc();
print("AFTER SECOND GC: was_closed="+was_closed);

Currently it prints:

before 9232, after 9232, break 0817b000
AFTER FIRST GC: was_closed=false
before 9232, after 9232, break 0817b000
AFTER SECOND GC: was_closed=true

That is, the generator was not closed after the loop and something continued to held its reference so it survived the first GC attempt. Only after extra explicit cleaning the second GC closed it.
Comment 1 Igor Bukanov 2006-08-19 11:31:07 PDT
I am morphing the bug. The real problem is that the current implementation of for-in loop delegates closing of the iterators to GC instead of closing the objects right after the loop. With native iterators one can get away with that as their close effects are not observable, but with generators that violates the promise that

for (i in iter)
    body;

should be equivalent to
try {
    for (i = iter.next(); ; i = iter.next())
        body;
} catch (e if e nstanceof StopIteration) {
} finally {
    iter.close();
}

The current implementation effectively lucks that finally block above and this is observable from scripts.

Fixing the bug would give another important benefit besides allowing for GC to collect the iterators faster. If running close hooks at arbitrary moment from GC would expose hard to address bugs, this feature can be disabled in FF2. 

With the bug addressed such last-resort measure would only affect scripts that explicitly call the next method on a generator and then cut the open generator with pending finally from the graph of live objects instead of simply calling generator.close(). Normal for-in users would not affected.
Comment 2 Brendan Eich [:brendan] 2006-08-26 10:48:01 PDT
Igor, can you take this one too?  I can help, I was going to use GC_MARK_DEBUG to see what ref kept the generator alive across one GC, but Jesse is keeping me busy with decompiler fun.

/be
Comment 3 Brendan Eich [:brendan] 2006-10-01 23:32:56 PDT
js> var was_closed = false;
js> function gen() {
        try {
                yield 1;
        } finally {
                was_closed = true;
        }
}
js> 
js> for (var i in gen())
        break;
js> 
js> gc();
before 9232, after 9232, break 01008000
js> 
js> print("AFTER FIRST GC: was_closed="+was_closed);
AFTER FIRST GC: was_closed=true
js> 
js> for (var i in (function() { yield 0; })) { }
js> gc();
before 9232, after 9232, break 01008000
js> print("AFTER SECOND GC: was_closed="+was_closed);
AFTER SECOND GC: was_closed=true

Fixed?  Or more to do here?

/be
Comment 4 Igor Bukanov 2006-10-02 02:00:32 PDT
(In reply to comment #3)
> js> gc();
> before 9232, after 9232, break 01008000
> js> print("AFTER SECOND GC: was_closed="+was_closed);
> AFTER SECOND GC: was_closed=true
> 
> Fixed?  Or more to do here?

The idea is to close without relying on GC whenever break is executed. It would even better if throwing an exception would also close the iterator. I.e.  

for (lvalue in obj)
  body;

should be equivalent to:

var iter = ValueToIterator(obj);
try {
    for (;;) {
        try {
           lvalue = iter.next(); 
        } catch (e if e instanceof StopIteration) {
            break;
        }   
    }
} finally {
    iter.close();
}

Currently that iter.close is executed only for native iterators and it is not executed when exceptions are thrown deferring closing to GC.  
Comment 5 Brendan Eich [:brendan] 2006-12-05 16:04:40 PST
This is "just" an optimization.  I don't see how it can possibly block 1.8.1.x. I'm not sure it should block bug 326466 either.  Thoughts on removing that blocks relationship?

/be
Comment 6 Jeff Thompson 2006-12-05 16:59:32 PST
(In reply to comment #5)
> This is "just" an optimization.  I don't see how it can possibly block 1.8.1.x.
> I'm not sure it should block bug 326466 either.  Thoughts on removing that
> blocks relationship?

My Python (and C#) code relies on the automatic closing of the iterator as
reported in this bug, especially when the iterator that needs to be closed
is "a couple levels down" in nested loops. So it is important to "implement Pythonic generators" as in 326466 for it to be built in to the language.
If not, then is there another way to force an iterator (and nested iterators)
to close?  (The original bug report mentions gc() but I don't think this works
in web page Javascript code.)
- Jeff
 

Comment 7 Jeff Walden [:Waldo] (remove +bmo to email) 2006-12-05 17:16:26 PST
(In reply to comment #6)
> If not, then is there another way to force an iterator (and nested iterators)
> to close?

gen.close() should work (which either throws GeneratorExit within the generator, or by returning within the generator -- the former was what was implemented first, but I think the latter is current behavior; handle either with a catch or finally), although you'll need some unsightly code to do so.

I disagree that this is "just" an optimization, but given that it's workaroundable I also can't see this blocking 1.8.1.1.
Comment 8 Jeff Thompson 2006-12-05 17:41:35 PST
(In reply to comment #7)
> gen.close() should work (which either throws GeneratorExit within the
> generator, or by returning within the generator -- the former was what was
> implemented first, but I think the latter is current behavior; handle either
> with a catch or finally), although you'll need some unsightly code to do so.

I assume that the "unsightly code" is to explicitly expand each "for (lvalue in obj)" into the 12 lines of code that Igor shows in comment #4.  The point is that
when you use an iterator, you don't know if has or if one of the nested iterators
that it calls has a try/finally block as shown in the original bug report.
(Imagine that you have another generator function that uses gen() and then
you use this generator function, etc.)
So all the levels of iterators would have to have the unsightly code in order
to ensure that the finally block of an inner iterator is properly called.  If this is the
workaround, then it is a error-prone and a high burden.
Comment 9 Igor Bukanov 2006-12-05 19:47:24 PST
(In reply to comment #5)
> This is "just" an optimization.  I don't see how it can possibly block 1.8.1.x.
> I'm not sure it should block bug 326466 either.  Thoughts on removing that
> blocks relationship?

This is not just an "optimization" but rather an observable behaviour. Thus I suggest to have this fixed at least on 1.8.1.* at some point and ideally for js1.7. The problem is how to do that efficiently without generating try/finally around each and every loop.

My current thinking is to have an extra iterator stack per frame that also records the loop PC. In this case the exception catching code can always close all the necessary iterators inside particular try/catch/finally. The good thing about it is that iterators on that stack can be removed from various GC tables removing the pressure from GC.

> 
> /be
> 

Comment 10 Brendan Eich [:brendan] 2006-12-05 21:44:09 PST
(In reply to comment #9)
> This is not just an "optimization" but rather an observable behaviour.

Optimization often has observable effects, but my point was that prompt close is not normative is ES4's draft spec.  Thus this is "at most" an optimization for portable code.  For SpiderMonkey-specific code, it could be "more".  See what I mean?

In any event, we should make sure the ES4 spec is "right", whatever that means. It involves compromises between implementation cost and usability or expressiveness, all over the place. So it's not likely to make anyone supremely happy.

/be
Comment 11 Brendan Eich [:brendan] 2006-12-05 22:00:59 PST
[I wrote this earlier this evening and almost lost it due to intermittent wireless and forgetfulness on my part. Posted for posterity, and to escalate any objections to the ES4 draft spec not requiring prompt close for iterators that die upon termination of their for-in loop. /be]

(In reply to comment #7)
> I disagree that this is "just" an optimization, but given that it's
> workaroundable I also can't see this blocking 1.8.1.1.

I wrote that "just" in quotes on purpose.  The current plan is that ECMA TG1 will not require prompt iterator close (meaning reference counting or some kind of scanning GC with escape analysis) as a normative part of the spec.  If this holds, portable script won't be able to count on such implementation-dependent behavior.

It's nice that Python has one dominant implementation, but JS does not, and it runs in constrained device environments and on many different runtimes -- and most of the JS in the world must be portable.

I'm sympathetic to the wish for prompt iterator close calling.  It's not "just" an optimization, divorced from the realities of JS and the web listed above, any more than proper tail calls are "just" an optimization (they're a stack allocation rule important for CPS).  But the realities bite.

BTW, ECMA 4 *will* require proper tail calls, because implementing them is less of a burden on implementations (some very small JS implementations use CPS and tail calls under the hood already), and they enable CPS programming, which has lots of winning uses in the browser's unthreaded execution model.

If anyone wants TG1 to revisit this, please mail the es4-discuss@mozilla.org list (subscribe first if you haven't; mailman protocol).

/be
Comment 12 Brendan Eich [:brendan] 2006-12-15 13:34:03 PST
Ok, ECMA TG1 members at today's meeting agreed to specify normative close after completion (normal or abrupt) of for-in loops and comprehensions, if the generator iterator can't escape.  So this bug should be fixed for JS1.8/Mozilla1.9/Firefox3. Thanks,

/be
Comment 13 Igor Bukanov 2007-05-16 15:47:02 PDT
Created attachment 265052 [details] [diff] [review]
implementation v0.1

Begining of the patch - this is just a backup and would not compile.
Comment 14 Igor Bukanov 2007-05-25 17:27:34 PDT
Created attachment 266151 [details] [diff] [review]
implementation v0.9

Here is quilt patch that depends on the patch for bug 381973 to be applied. It passes the test suite without regressions and the following test case.
Comment 15 Igor Bukanov 2007-05-25 17:30:51 PDT
Created attachment 266152 [details]
test case for generator close

Here is a test case for various cases of generator and control flow jumps interactions.
Comment 16 Igor Bukanov 2007-06-05 11:20:18 PDT
Created attachment 267298 [details] [diff] [review]
implementation v1

Comments on the patch:

1. There is a new try note to close the iterator in presence of exceptions. To simplify things I changed the emitter to build try notes from a linked list instead of using preallocated array. Otherwise it would require to track in many places in parser where for-in loops can happen tryNoteCount. And it is easy to miss it.

2. Now enditer can execute an arbitrary code via finally in the generator. That requires to take care of not executing finally/enditer when enditer is generated as a part of break/continue/return inside deeply nested try/for-in structure. In the patch I done that via an extra check for a stack depth to reject already executed finally/enditer.

3. There are some renames in jsiter.c to emphasis the fact that code called from [enditer] closes anything closeable.
Comment 17 Igor Bukanov 2007-06-12 10:03:12 PDT
review ping...
Comment 18 Brendan Eich [:brendan] 2007-06-12 15:25:49 PDT
Doing bug 384151 review first.

/be
Comment 19 Brendan Eich [:brendan] 2007-06-12 18:41:29 PDT
Comment on attachment 267298 [details] [diff] [review]
implementation v1

>+/*
>+ * objp parameter is kept only for compatibility. If non-null, on return it
>+ * contains obj.
>+ */
> extern JS_PUBLIC_API(JSBool)
> JS_GetMethodById(JSContext *cx, JSObject *obj, jsid id, JSObject **objp,
>                  jsval *vp);
> 
>+/*
>+ * objp parameter is kept only for compatibility. If non-null, on return it
>+ * contains obj.
>+ */
> extern JS_PUBLIC_API(JSBool)
> JS_GetMethod(JSContext *cx, JSObject *obj, const char *name, JSObject **objp,
>              jsval *vp);

Separate patch? Just wondering if these are worth splitting out.

>@@ -1326,35 +1330,37 @@ EmitNonLocalJumpFixup(JSContext *cx, JSC
>                       JSOp *returnop)
> {
>     intN depth, npops;
>     JSStmtInfo *stmt;
>     ptrdiff_t jmp;
> 
>     /*
>-     * Return from within a try block that has a finally clause must be split
>-     * into two ops: JSOP_SETRVAL, to pop the r.v. and store it in fp->rval;
>-     * and JSOP_RETRVAL, which makes control flow go back to the caller, who
>-     * picks up fp->rval as usual.  Otherwise, the stack will be unbalanced
>-     * when executing the finally clause.
>+     * Return from within a try block that has a finally clause or from for-in

Consistent style would use "a for-in", to match "a try block" and "a finally clause".

>+     * loopmust be split into two ops: JSOP_SETRVAL, to pop the r.v. and store

"loop must" ran together above.

>      * We mutate *returnop once only if we find an enclosing try-block (viz,
>-     * STMT_FINALLY) to ensure that we emit just one JSOP_SETRVAL before one
>-     * or more JSOP_GOSUBs and other fixup opcodes emitted by this function.
>+     * STMT_FINALLY) or for-in loop to ensure that we emit just one

"a for-in loop" nit again.

>+     * JSOP_SETRVAL before one or more JSOP_GOSUBs/JSOP_ENDTERs and other

ENDITER missing the I.

>+     * fixup opcodes emitted by this function.

Extra blank comment line here?

>@@ -4734,16 +4736,24 @@ js_EmitTree(JSContext *cx, JSCodeGenerat
>         }
> 
>         /* Now fixup all breaks and continues (before for/in's JSOP_ENDITER). */
>         if (!js_PopStatementCG(cx, cg))
>             return JS_FALSE;
> 
>         if (pn2->pn_type == TOK_IN) {
>-            if (js_Emit1(cx, cg, JSOP_ENDITER) < 0)
>+            /*
>+             * enditer needs a slot to save an exception throws from the body

Nit: JSOP_ENDITER not enditer.

s/throws/thrown/


> void
> js_FinishTakingTryNotes(JSContext *cx, JSCodeGenerator *cg,
>                         JSTryNoteArray *array)
> {
>-    JS_ASSERT(cg->tryNext - cg->tryBase == (ptrdiff_t) array->length);
>-    memcpy(array->notes, cg->tryBase, TRYNOTE_SIZE(array->length));
>+    JSTryNoteNode *tnn;
>+    JSTryNote *tn;
>+
>+    JS_ASSERT(array->length > 0 && array->length == cg->ntrynotes);
>+    tn = array->notes + array->length;
>+    tnn = cg->lastTryNote;
>+    do {
>+        --tn;
>+        *tn = tnn->note;

Nit: *--tn = ... is fine by me and the Unix creators one of whom created C as well ;-).


>+typedef struct JSTryNoteNode JSTryNoteNode;

Nit: blank line here.

>+struct JSTryNoteNode {
>+    JSTryNote       note;
>+    JSTryNoteNode   *prev;
>+};
[snip]
>@@ -5999,20 +5996,20 @@ interrupt:
>         }
> #endif /* DEBUG */
>     }
> #endif /* !JS_THREADED_INTERP */
> 
> out:
>     JS_ASSERT((size_t)(pc - script->code) < script->length);
>-    if (!ok) {
>+    if (!ok && cx->throwing && !(fp->flags & JSFRAME_FILTERING)) {
>         /*
>-         * Has an exception been raised?  Also insist that we are not in an
>-         * XML filtering predicate expression, to avoid catching exceptions
>-         * within the filtering predicate, such as this example taken from
>-         * tests/e4x/Regress/regress-301596.js:
>+         * Exception has been raised and we are not in an XML filtering

Nit: "An exception has been raised..."

>+         * predicate expression. The latter check is necessary to  to avoid

Doubled "to" and extra spaces.

[snip]
A diff -w for the changes leading up to the following would be helpful.

>+                /*
>+                 * We have a note that covers the exception pc but we need to
>+                 * check if the interpreter has not already executed the
>+                 * corresponding note handler. This is possible when the

s/note handler/handler/ or something like that -- on second thought, both uses of "not" in the three lines above should probably by "try" instead.

>+                 * executed bytecode implements break or return from inside a
>+                 * for-in loop.
>+                 *
>+                 * In this case the emitter generates additional [enditer] and
>+                 * [gosub] to close all outstanding iterators and execute the

Nit: "opcodes" after "[gosub]".

>+                 * finally blocks. If such [enditer] throws an exception, its

"such an [enditer]"

>+                 * pc can still be inside several nested for-in loops and
>+                 * try-with-finally statements even if we have already closed

Canonical phrase is try-finally not try-with-finally.

>+                 * the corresponding iterators and invoked the finally blocks.
>+                 *
>+                 * To address this, we make [enditer] to always decrease the

s/to always/always/

>+                 * stack even when its implementation throws an exception.
>+                 * Thus already executed [enditer] and [gosub] will have try

opcodes after [gosub] again

>+                 * notes with the stack depth exceeding the current one and
>+                 * this what we use to filter them out.

"this condition"

>+JSBool
>+js_CloseIterator(JSContext *cx, jsval v)
>+{
>+    JSObject *obj;
>+    JSClass *clasp;
>+
>+    JS_ASSERT(!JSVAL_IS_PRIMITIVE(v));
>+    obj = JSVAL_TO_OBJECT(v);
>+    clasp = OBJ_GET_CLASS(cx, obj);
>+
>+    if (clasp == &js_IteratorClass) {
>+        js_CloseNativeIterator(cx, obj);
>+    }

Nit: overbraced -- house style is lenient when #if* intervenes. Also:

>+#if JS_HAS_GENERATORS
>+    else if (clasp == &js_GeneratorClass) {
>+        if (!js_CloseGenerator(cx, obj))
>+            return JS_FALSE;
>+    }

Could use if && instead of if if.

>+#endif
>+    return JS_TRUE;
>+}
>+
> static JSBool
> CallEnumeratorNext(JSContext *cx, JSObject *iterobj, uintN flags, jsval *rval)
> {
>     JSObject *obj, *origobj;
>     jsval state;
>     JSBool foreach;
>     jsid id;
>@@ -599,15 +598,15 @@ js_CallIteratorNext(JSContext *cx, JSObj
>          * read-only and permanent.
>          */
>         if (!IteratorNextImpl(cx, iterobj, rval))
>             return JS_FALSE;
>     } else {
>         jsid id = ATOM_TO_JSID(cx->runtime->atomState.nextAtom);
> 
>-        if (!JS_GetMethodById(cx, iterobj, id, &iterobj, rval))
>+        if (!JS_GetMethodById(cx, iterobj, id, NULL, rval))
>             return JS_FALSE;
>         if (!js_InternalCall(cx, iterobj, *rval, 0, NULL, rval)) {
>             /* Check for StopIteration. */
>             if (!cx->throwing ||
>                 JSVAL_IS_PRIMITIVE(cx->exception) ||
>                 OBJ_GET_CLASS(cx, JSVAL_TO_OBJECT(cx->exception))
>                     != &js_StopIterationClass) {
>@@ -907,18 +906,31 @@ SendToGenerator(JSContext *cx, JSGenerat
> }
> 
> /*
>  * Execute gen's close hook after the GC detects that the object has become
>  * unreachable.
>  */
> JSBool
>-js_CloseGeneratorObject(JSContext *cx, JSGenerator *gen)
>+js_CloseGenerator(JSContext *cx, JSObject *genobj)
> {
>+    JSGenerator *gen;
>+
>+    JS_ASSERT(STOBJ_GET_CLASS(genobj) == &js_GeneratorClass);
>+    gen = (JSGenerator *) JS_GetPrivate(cx, genobj);
>+    if (!gen) {
>+        /* Generator prototype object. */
>+        return JS_TRUE;
>+    }
>+
>+    JS_ASSERT(gen->state != JSGEN_RUNNING && gen->state != JSGEN_CLOSING);
>+    if (gen->state == JSGEN_CLOSED)
>+        return JS_TRUE;
>+
>     /* We pass null as rval since SendToGenerator never uses it with CLOSE. */
>-    return SendToGenerator(cx, JSGENOP_CLOSE, gen->obj, gen, JSVAL_VOID, NULL);
>+    return  SendToGenerator(cx, JSGENOP_CLOSE, genobj, gen, JSVAL_VOID, NULL);
> }
> 
> /*
>  * Common subroutine of generator_(next|send|throw|close) methods.
>  */
> static JSBool
> generator_op(JSContext *cx, JSGeneratorOp op,
>Index: js/src/jsiter.h
>===================================================================
>RCS file: /cvsroot/mozilla/js/src/jsiter.h,v
>retrieving revision 3.16
>diff -U7 -p -r3.16 jsiter.h
>--- js/src/jsiter.h	5 Oct 2006 00:19:49 -0000	3.16
>+++ js/src/jsiter.h	5 Jun 2007 17:59:18 -0000
>@@ -45,36 +45,39 @@
> #include "jsprvtd.h"
> #include "jspubtd.h"
> 
> #define JSITER_ENUMERATE  0x1   /* for-in compatible hidden default iterator */
> #define JSITER_FOREACH    0x2   /* return [key, value] pair rather than key */
> #define JSITER_KEYVALUE   0x4   /* destructuring for-in wants [key, value] */
> 
>-extern void
>-js_CloseNativeIterator(JSContext *cx, JSObject *iterobj);
>-
>-extern void
>-js_CloseIteratorState(JSContext *cx, JSObject *iterobj);
>-
> /*
>  * Convert the value stored in *vp to its iteration object. The flags should
>  * contain JSITER_ENUMERATE if js_ValueToIterator is called when enumerating
>  * for-in semantics are required, and when the caller can guarantee that the
>  * iterator will never be exposed to scripts.
>  */
> extern JSBool
> js_ValueToIterator(JSContext *cx, uintN flags, jsval *vp);
> 
>+extern JSBool
>+js_CloseIterator(JSContext *cx, jsval v);
>+
> /*
>  * Given iterobj, call iterobj.next().  If the iterator stopped, set *rval to
>  * JSVAL_HOLE. Otherwise set it to the result of the next call.
>  */
> extern JSBool
> js_CallIteratorNext(JSContext *cx, JSObject *iterobj, jsval *rval);
> 
>+/*
>+ * Close iterator when its class is js_IteratorClass.
>+ */
>+extern void
>+js_CloseNativeIterator(JSContext *cx, JSObject *iterobj);
>+
> #if JS_HAS_GENERATORS
> 
> /*
>  * Generator state codes.
>  */
> typedef enum JSGeneratorState {
>     JSGEN_NEWBORN,  /* not yet started */
>@@ -96,15 +99,15 @@ struct JSGenerator {
> #define FRAME_TO_GENERATOR(fp) \
>     ((JSGenerator *) ((uint8 *)(fp) - offsetof(JSGenerator, frame)))
> 
> extern JSObject *
> js_NewGenerator(JSContext *cx, JSStackFrame *fp);
> 
> extern JSBool
>-js_CloseGeneratorObject(JSContext *cx, JSGenerator *gen);
>+js_CloseGenerator(JSContext *cx, JSObject *genobj);
> 
> #endif
> 
> extern JSClass          js_GeneratorClass;
> extern JSClass          js_IteratorClass;
> extern JSClass          js_StopIterationClass;
> 
>Index: js/src/jsopcode.tbl
>===================================================================
>RCS file: /cvsroot/mozilla/js/src/jsopcode.tbl,v
>retrieving revision 3.96
>diff -U7 -p -r3.96 jsopcode.tbl
>--- js/src/jsopcode.tbl	5 Jun 2007 03:07:38 -0000	3.96
>+++ js/src/jsopcode.tbl	5 Jun 2007 17:59:18 -0000
>@@ -461,15 +461,15 @@ OPDEF(JSOP_LOCALINC,      205,"localinc"
> OPDEF(JSOP_LOCALDEC,      206,"localdec",    NULL,    3,  0,  1, 15,  JOF_LOCAL|JOF_NAME|JOF_DEC|JOF_POST)
> OPDEF(JSOP_FORLOCAL,      207,"forlocal",    NULL,    3,  0,  1, 19,  JOF_LOCAL|JOF_NAME|JOF_FOR)
> 
> /*
>  * Iterator, generator, and array comprehension support.
>  */
> OPDEF(JSOP_FORCONST,      208,"forconst",    NULL,    3,  0,  1, 19,  JOF_QVAR|JOF_NAME|JOF_FOR)
>-OPDEF(JSOP_ENDITER,       209,"enditer",     NULL,    1,  1,  0,  0,  JOF_BYTE)
>+OPDEF(JSOP_ENDITER,       209,"enditer",     NULL,    1,  1,  0,  0,  JOF_BYTE|JOF_TMPSLOT)
> OPDEF(JSOP_GENERATOR,     210,"generator",   NULL,    1,  0,  0,  0,  JOF_BYTE)
> OPDEF(JSOP_YIELD,         211,"yield",       NULL,    1,  1,  1,  1,  JOF_BYTE)
> OPDEF(JSOP_ARRAYPUSH,     212,"arraypush",   NULL,    3,  1,  0,  3,  JOF_LOCAL)
> 
> OPDEF(JSOP_FOREACHKEYVAL, 213,"foreachkeyval",NULL,   1,  1,  1,  0,  JOF_BYTE)
> 
> /*
>Index: js/src/jsparse.c
>===================================================================
>RCS file: /cvsroot/mozilla/js/src/jsparse.c,v
>retrieving revision 3.288
>diff -U7 -p -r3.288 jsparse.c
>--- js/src/jsparse.c	4 Jun 2007 23:03:03 -0000	3.288
>+++ js/src/jsparse.c	5 Jun 2007 17:59:20 -0000
>@@ -1470,15 +1470,14 @@ FunctionDef(JSContext *cx, JSTokenStream
>         op = JSOP_NOP;
>     }
> 
>     pn->pn_funAtom = objAtom;
>     pn->pn_op = op;
>     pn->pn_body = body;
>     pn->pn_flags = funtc.flags & (TCF_FUN_FLAGS | TCF_HAS_DEFXMLNS);
>-    pn->pn_tryCount = funtc.tryCount;
>     TREE_CONTEXT_FINISH(&funtc);
>     return result;
> }
> 
> static JSParseNode *
> FunctionStmt(JSContext *cx, JSTokenStream *ts, JSTreeContext *tc)
> {
>@@ -1545,15 +1544,14 @@ Statements(JSContext *cx, JSTokenStream 
>                 /*
>                  * Clear TCF_RETURN_EXPR so FunctionBody doesn't try to
>                  * CheckFinalReturn again.
>                  */
>                 tc->flags &= ~TCF_RETURN_EXPR;
>             }
>             if (!js_FoldConstants(cx, pn2, tc) ||
>-                !js_AllocTryNotes(cx, (JSCodeGenerator *)tc) ||
>                 !js_EmitTree(cx, (JSCodeGenerator *)tc, pn2)) {
>                 tt = TOK_ERROR;
>                 break;
>             }
>             RecycleTree(pn2, tc);
>         } else {
>             PN_APPEND(pn, pn2);
>@@ -3181,15 +3179,14 @@ Statement(JSContext *cx, JSTokenStream *
>                 tt = js_GetToken(cx, ts);
>                 ts->flags &= ~TSF_OPERAND;
>             } while (tt == TOK_CATCH);
>         }
>         pn->pn_kid2 = catchList;
> 
>         if (tt == TOK_FINALLY) {
>-            tc->tryCount++;
>             MUST_MATCH_TOKEN(TOK_LC, JSMSG_CURLY_BEFORE_FINALLY);
>             js_PushStatement(tc, &stmtInfo, STMT_FINALLY, -1);
>             pn->pn_kid3 = Statements(cx, ts, tc);
>             if (!pn->pn_kid3)
>                 return NULL;
>             MUST_MATCH_TOKEN(TOK_RC, JSMSG_CURLY_AFTER_FINALLY);
>             js_PopStatement(tc);
>@@ -3197,15 +3194,14 @@ Statement(JSContext *cx, JSTokenStream *
>             js_UngetToken(ts);
>         }
>         if (!catchList && !pn->pn_kid3) {
>             js_ReportCompileErrorNumber(cx, ts, JSREPORT_TS | JSREPORT_ERROR,
>                                         JSMSG_CATCH_OR_FINALLY);
>             return NULL;
>         }
>-        tc->tryCount++;
>         return pn;
>       }
> 
>       case TOK_THROW:
>         pn = NewParseNode(cx, ts, PN_UNARY, tc);
>         if (!pn)
>             return NULL;
>@@ -4290,14 +4286,15 @@ ComprehensionTail(JSContext *cx, JSToken
>     JSParseNode *pn, *pn2, *pn3, **pnp;
>     JSStmtInfo stmtInfo;
>     BindData data;
>     JSRuntime *rt;
>     JSTokenType tt;
>     JSAtom *atom;
> 
>+    JS_ASSERT(type == TOK_SEMI || type == TOK_ARRAYPUSH);
>     JS_ASSERT(CURRENT_TOKEN(ts).type == TOK_FOR);
> 
>     /*
>      * Make a parse-node and literal object representing the block scope of
>      * this array comprehension or generator expression.
>      */
>     pn = PushLexicalScope(cx, ts, tc, &stmtInfo);
>Index: js/src/jsparse.h
>===================================================================
>RCS file: /cvsroot/mozilla/js/src/jsparse.h,v
>retrieving revision 3.41
>diff -U7 -p -r3.41 jsparse.h
>--- js/src/jsparse.h	18 May 2007 01:41:17 -0000	3.41
>+++ js/src/jsparse.h	5 Jun 2007 17:59:20 -0000
>@@ -65,14 +65,15 @@ JS_BEGIN_EXTERN_C
>  *                            arg and var properties.  We create the function
>  *                            object at parse (not emit) time to specialize arg
>  *                            and var bytecodes early.
>  *                          pn_body: TOK_LC node for function body statements
>  *                          pn_flags: TCF_FUN_* flags (see jsemit.h) collected
>  *                            while parsing the function's body
>  *                          pn_tryCount: of try statements in function
>+ *                            including implied ones required by for-in loop.

Isn't pn_tryCount dead now? Just delete these two comment lines.

I'll stamp a new patch if this all tests out well in js/tests and Firefox.

/be
Comment 20 Brendan Eich [:brendan] 2007-06-12 18:52:49 PDT
Yikes -- sorry for not cutting out a bunch of the patch in that last comment.

Also, I'm noting here that comment 12 is obsolete -- the proposal at http://developer.mozilla.org/es4/proposals/iterators_and_generators.html now says that close is automated *only* for a generator started by a for-in loop.

The precise meaning of "started by a for-in loop" TBD, but this gets rid of finalization as a part of the ES4 spec altogether, which is winning.

/be
Comment 21 Igor Bukanov 2007-06-14 01:04:25 PDT
(In reply to comment #19)
> (From update of attachment 267298 [details] [diff] [review])
> >+/*
> >+ * objp parameter is kept only for compatibility. If non-null, on return it
> >+ * contains obj.
> >+ */
> > extern JS_PUBLIC_API(JSBool)
> > JS_GetMethodById(JSContext *cx, JSObject *obj, jsid id, JSObject **objp,
> >                  jsval *vp);
> Separate patch? Just wondering if these are worth splitting out.

See bug 384396. Those changes are indeed 100% orthogonal to this bug.

> >+JSBool
> >+js_CloseIterator(JSContext *cx, jsval v)
> >+{
> >+    JSObject *obj;
> >+    JSClass *clasp;
> >+
> >+    JS_ASSERT(!JSVAL_IS_PRIMITIVE(v));
> >+    obj = JSVAL_TO_OBJECT(v);
> >+    clasp = OBJ_GET_CLASS(cx, obj);
> >+
> >+    if (clasp == &js_IteratorClass) {
> >+        js_CloseNativeIterator(cx, obj);
> >+    }
> 
> Nit: overbraced -- house style is lenient when #if* intervenes. Also:
> 
> >+#if JS_HAS_GENERATORS
> >+    else if (clasp == &js_GeneratorClass) {
> >+        if (!js_CloseGenerator(cx, obj))
> >+            return JS_FALSE;
> >+    }
> 
> Could use if && instead of if if.

Consider the full source:

    if (clasp == &js_IteratorClass) {
        js_CloseNativeIterator(cx, obj);
    }
#if JS_HAS_GENERATORS
    else if (clasp == &js_GeneratorClass) {
        if (!js_CloseGenerator(cx, obj))
            return JS_FALSE;
    }
#endif
    return JS_TRUE;

The initial version of the patch had an else clause to execute close method on the a generic iterator, but I removed that since it is unrelated to this bug. But I argue that even without that else this is not overbraced as if one removes the preprocessor macros, then the result would be:

    if (clasp == &js_IteratorClass) {
        js_CloseNativeIterator(cx, obj);
    } else if (clasp == &js_GeneratorClass) {
        if (!js_CloseGenerator(cx, obj))
            return JS_FALSE;
    }
    return JS_TRUE;

This is not overbraced if one keeps the third if inside else if. But that if is rather natural since the whole structure is a switch over clasp.
Comment 22 Brendan Eich [:brendan] 2007-06-14 10:20:48 PDT
Don't sweat that brace nit too much. I was mainly thinking of the then clause. The else-if-if is ok.

/be
Comment 23 Igor Bukanov 2007-06-14 12:48:58 PDT
Created attachment 268403 [details] [diff] [review]
implementation v2

The new version removes GetMethod-related changes and addresses the nits.
Comment 24 Igor Bukanov 2007-06-14 12:51:01 PDT
Created attachment 268404 [details] [diff] [review]
implementation v2 (diff -b version)
Comment 25 Igor Bukanov 2007-06-14 12:53:04 PDT
Created attachment 268405 [details]
diff between v1 and v2
Comment 26 Igor Bukanov 2007-06-25 08:19:08 PDT
ping for review...
Comment 27 Brendan Eich [:brendan] 2007-06-25 13:36:01 PDT
Comment on attachment 268403 [details] [diff] [review]
implementation v2

from within => from
JS_ENDITER
JSTryNoteNode => JSTryNode
lastTryNote => lastTryNode

doubled space after return:
    return  SendToGenerator(cx, JSGENOP_CLOSE, genobj, gen, JSVAL_VOID, NULL);

can not => cannot in:
                    /* Catch can not intercept the closing of a generator. */

comment lies about false:

                     * Push (false, exception) pair for finally to indicate
                     * that [retsub] should rethrow the exception.
                     */
                    PUSH(JSVAL_TRUE);

typos in comment (interpretr, tyhe) and "now use" should be "now uses":
                     * This almost duplicates JSOP_ENDITER in the interpretr
                     * loop except the code now use a reserved stack slot to
                     * save tyhe exception.

just below:

                    SAVE_SP_AND_PC(fp);
                    ok = js_CloseIterator(cx, sp[-2]);
                    if (ok) {
                        cx->throwing = JS_TRUE;
                        cx->exception = sp[-1];
                        ok = JS_FALSE;
                    }
                    SAVE_SP_AND_PC(fp);
                    sp -= 2;
                    if (!ok)
                        goto out;
                    break;

Why the two SAVE_SP_AND_PC calls? If js_CloseIterator changed fp->sp, we would want RESTORE_SP at most. In any event we should not be re-fetching or -storing pc.

Comment below should say "Close iterobj, whose class must be js_IteratorClass."

/*
 * Close iterator when its class is js_IteratorClass.
 */
extern void
js_CloseNativeIterator(JSContext *cx, JSObject *iterobj);
Nit: "genobj" could be "obj" in

js_CloseGenerator(JSContext *cx, JSObject *genobj);

and its definition.

I'll stamp the next one promptly.

/be
Comment 28 Brendan Eich [:brendan] 2007-06-25 13:37:27 PDT
(In reply to comment #27)
> (From update of attachment 268403 [details] [diff] [review])
> from within => from
> JS_ENDITER
> JSTryNoteNode => JSTryNode
> lastTryNote => lastTryNode

In case this wasn't clear (I meant to show more context ;-), find the left-hand string and replace with right, re-wrapping. First two are one-offs, the second two are global search/replace.

/be
Comment 29 Igor Bukanov 2007-06-26 00:56:55 PDT
(In reply to comment #27)
> (From update of attachment 268403 [details] [diff] [review])
> from within => from
> JS_ENDITER

But what exactly the problem with JS_ENDITER? I hope this is not a suggestion to nuke it from sources ;)
Comment 30 Brendan Eich [:brendan] 2007-06-26 01:13:47 PDT
(In reply to comment #29)
> (In reply to comment #27)
> > (From update of attachment 268403 [details] [diff] [review] [details])
> > from within => from
> > JS_ENDITER
> 
> But what exactly the problem with JS_ENDITER? I hope this is not a suggestion
> to nuke it from sources ;)

JS_ENDITER => JSOP_ENDITER

/be
Comment 31 Igor Bukanov 2007-06-26 03:36:39 PDT
Created attachment 269834 [details] [diff] [review]
implementation v3

Here is a new patch to address the nits, remove the double SAVE_SP_AND_PC in JSTN_ITER case and just continue the handler search loop when js_CloseIterator returns true.
Comment 32 Igor Bukanov 2007-06-26 03:40:34 PDT
Created attachment 269835 [details]
 diff between v2 and v3

Bugzilla can not show the differences in jsinter.c, so here is the manual diff.
Comment 33 Igor Bukanov 2007-06-28 11:45:36 PDT
(In reply to comment #27)
> I'll stamp the next one promptly.

Here is a prompt note ;)
Comment 34 Brendan Eich [:brendan] 2007-06-28 18:01:46 PDT
Comment on attachment 269834 [details] [diff] [review]
implementation v3

>+         do {
>+             if (offset - tn->start >= tn->length)
>+                 continue;
>+
>+                 /*

Code from here on is overindented one indentation level.

>+                  * We have a note that covers the exception pc but we need to
>+                  * check if the interpreter has not already executed the

Suggest "whether the interpreter has already executed" instead.


>+                 for (obj = fp->scopeChain;
>+                      (clasp = OBJ_GET_CLASS(cx, obj)) == &js_WithClass ||
>+                          clasp == &js_BlockClass;

The ||'s right operand is not indented to line up with its left.

>+                     ok = js_CloseIterator(cx, sp[-2]);
>+                     if (!ok) {
>+                         /*
>+                          * close generated a new exception, restart the
>+                          * handler search to properly notify the debugger.
>+                          */
>+                         sp -= 2;
>+                         goto out;
>+                     }
>+                     cx->throwing = JS_TRUE;
>+                     cx->exception = sp[-1];
>+                     sp -= 2;

Can't you common the sp -= 2 before the if?

r=me with these fixed.

/be
Comment 35 Igor Bukanov 2007-07-01 16:32:11 PDT
Created attachment 270528 [details] [diff] [review]
implementation v3b

Patch to commit with nits addressed.
Comment 36 Igor Bukanov 2007-07-02 05:14:40 PDT
I committed the patch from comment 35 to the trunk:

Checking in js.c;
/cvsroot/mozilla/js/src/js.c,v  <--  js.c
new revision: 3.153; previous revision: 3.152
done
Checking in jsemit.c;
/cvsroot/mozilla/js/src/jsemit.c,v  <--  jsemit.c
new revision: 3.261; previous revision: 3.260
done
Checking in jsemit.h;
/cvsroot/mozilla/js/src/jsemit.h,v  <--  jsemit.h
new revision: 3.76; previous revision: 3.75
done
Checking in jsgc.c;
/cvsroot/mozilla/js/src/jsgc.c,v  <--  jsgc.c
new revision: 3.222; previous revision: 3.221
done
Checking in jsinterp.c;
/cvsroot/mozilla/js/src/jsinterp.c,v  <--  jsinterp.c
new revision: 3.355; previous revision: 3.354
done
Checking in jsiter.c;
/cvsroot/mozilla/js/src/jsiter.c,v  <--  jsiter.c
new revision: 3.60; previous revision: 3.59
done
Checking in jsiter.h;
/cvsroot/mozilla/js/src/jsiter.h,v  <--  jsiter.h
new revision: 3.17; previous revision: 3.16
done
Checking in jsopcode.tbl;
/cvsroot/mozilla/js/src/jsopcode.tbl,v  <--  jsopcode.tbl
new revision: 3.97; previous revision: 3.96
done
Checking in jsparse.c;
/cvsroot/mozilla/js/src/jsparse.c,v  <--  jsparse.c
new revision: 3.290; previous revision: 3.289
done
Checking in jsparse.h;
/cvsroot/mozilla/js/src/jsparse.h,v  <--  jsparse.h
new revision: 3.42; previous revision: 3.41
done
Checking in jsscript.c;
/cvsroot/mozilla/js/src/jsscript.c,v  <--  jsscript.c
new revision: 3.147; previous revision: 3.146
done
Checking in jsscript.h;
/cvsroot/mozilla/js/src/jsscript.h,v  <--  jsscript.h
new revision: 3.36; previous revision: 3.35
done
Comment 37 Bob Clary [:bc:] 2007-07-13 10:24:16 PDT
/cvsroot/mozilla/js/tests/js1_8/genexps/regress-349326.js,v  <--  regress-349326.js
initial revision: 1.1
Comment 38 Bob Clary [:bc:] 2007-09-18 15:01:38 PDT
verified fixed 1.9.0 linux/mac*/windows.

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