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.

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.

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

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

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

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

(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

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

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

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

(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

[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

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

Begining of the patch - this is just a backup and would not compile.

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.

Here is a test case for various cases of generator and control flow jumps interactions.

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.

review ping...

Doing bug 384151 review first. /be

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

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

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

Don't sweat that brace nit too much. I was mainly thinking of the then clause. The else-if-if is ok. /be

The new version removes GetMethod-related changes and addresses the nits.

ping for review...

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

(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

(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 ;)

(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

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.

Bugzilla can not show the differences in jsinter.c, so here is the manual diff.

(In reply to comment #27) > I'll stamp the next one promptly. Here is a prompt note ;)

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

Patch to commit with nits addressed.

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

/cvsroot/mozilla/js/tests/js1_8/genexps/regress-349326.js,v <-- regress-349326.js initial revision: 1.1

verified fixed 1.9.0 linux/mac*/windows.