ES4 dropped the requirement to close the generators when they become unreachable while yielding inside try with finally. Currently SpiderMonkey supports it, but in the browser embedding in a lot of specific cases the outstanding finally blocks are not executed for security reasons. These special cases make it impossible for a script author to rely on the feature. Thus the idea is to remove the feature altogether simplifying GC while making the generator behavior uniform. But such removal requires that the generators are always closed when executed via for-in loop even in the presence of exceptions meaning that this bug depends on 349326.

Will this fix bug 355053?

(In reply to comment #1) > Will this fix bug 355053? Yes. /be

The patch pretty much consists of minuses removing close phase code.

Comment on attachment 271984 [details] [diff] [review] implementation v1 Whew -- that lightened the load! Thanks, /be

With the patch applied the following test cases that relies on the close to be called from GC fail: js1_7/geniter/regress-347739.js js1_7/geniter/regress-349331.js js1_7/geniter/regress-349012-01.js js1_7/iterable/regress-340526-02.js This patch fixes the first 3 tests to use autoclosing of generator after for-in loop. The forth test has no replacement and should be removed as it tested a very specific interaction between GC and close hooks that no longer exists with the patch.

(In reply to comment #6) > Created an attachment (id=272076) [details] > test suite update To be precise: the altered test cases allows to continue to test for their regressions with or without support of calling close on GC. Thus fixing this bug should not change the test results for them.

I checked in the patch from comment 3 to the trunk: Checking in jsapi.c; /cvsroot/mozilla/js/src/jsapi.c,v <-- jsapi.c new revision: 3.337; previous revision: 3.336 done Checking in jscntxt.h; /cvsroot/mozilla/js/src/jscntxt.h,v <-- jscntxt.h new revision: 3.153; previous revision: 3.152 done Checking in jsgc.c; /cvsroot/mozilla/js/src/jsgc.c,v <-- jsgc.c new revision: 3.226; previous revision: 3.225 done Checking in jsgc.h; /cvsroot/mozilla/js/src/jsgc.h,v <-- jsgc.h new revision: 3.72; previous revision: 3.71 done Checking in jsiter.c; /cvsroot/mozilla/js/src/jsiter.c,v <-- jsiter.c new revision: 3.62; previous revision: 3.61 done Checking in jsiter.h; /cvsroot/mozilla/js/src/jsiter.h,v <-- jsiter.h new revision: 3.19; previous revision: 3.18 done

fxdbug-linux-tbox shows that after the patch reduced A-count to 650K, the best value for a few days while increasing Lk slightly to 1.25MB. That decrease in A is rather strange unless something creates a lot of yielding generators.

The last patch was incomplete: there is more to remove.

Fixing bad indentation in the last patch.

Isn't this still the case? * If a generator-iterator's arguments or call object escapes, it needs to * mark its generator object. /be

(In reply to comment #13) > Isn't this still the case? > > * If a generator-iterator's arguments or call object escapes, it needs to > * mark its generator object. See bug 355486. /be

(In reply to comment #14) > (In reply to comment #13) > > Isn't this still the case? > > > > * If a generator-iterator's arguments or call object escapes, it needs to > > * mark its generator object. > > See bug 355486. That was too zealous code removal, thanks for sporting this. The patch must mark gen struct data that is currently done in generator_trace.

New patch that fixing bad oversite ib the previous one.

(In reply to comment #16) > New patch that fixing bad oversite ib the previous one. Too fast typing, unencrypted message was "New patch that fixes the bad oversight in the previous one"

Comment on attachment 272217 [details] [diff] [review] Extra removals of no longer used code/fields v2 Nice, r=me. /be

Patch to check in with "extern" removed from js_TraceGenerator definition.

The previous patch had "void *", not "void" as a return type for js_TraceGenerator.

I checked in the patch from comment 20 to the trunk: Checking in jsfun.c; /cvsroot/mozilla/js/src/jsfun.c,v <-- jsfun.c new revision: 3.201; previous revision: 3.200 done Checking in jsiter.c; /cvsroot/mozilla/js/src/jsiter.c,v <-- jsiter.c new revision: 3.63; previous revision: 3.62 done Checking in jsiter.h; /cvsroot/mozilla/js/src/jsiter.h,v <-- jsiter.h new revision: 3.20; previous revision: 3.19 done

(In reply to comment #6) > js1_7/geniter/regress-349331.js > js1_7/geniter/regress-349012-01.js with this patch, these tests fail on the 1.8 branch. Is there anyway to make the test work in both 1.8 and trunk or do we need separate tests?

(In reply to comment #22) > (In reply to comment #6) > > > js1_7/geniter/regress-349331.js > > js1_7/geniter/regress-349012-01.js > > with this patch, these tests fail on the 1.8 branch. Is there anyway to make > the test work in both 1.8 and trunk or do we need separate tests? The patched test relies on bug 349326 being fixed. That bug blocks JS1.7 and the original goal almost a year ago was to include a fix in 1.8.1.*. I think this is still desirable so I will ask for a bug and its dependency for inclusion on the branch. If that would be rejected, then I patch the test once again not to depend on bug 349326 and this bug for the price of lesser coverage.

(In reply to comment #23) > If that would be rejected, then I patch the test once > again not to depend on bug 349326 and this bug for the price of lesser > coverage. Would prefer to keep as many tests as js1_8 tests as possible and just not run JS1.8 tests against the branch. /be

I baked out the patch from comment 20: Checking in jsfun.c; /cvsroot/mozilla/js/src/jsfun.c,v <-- jsfun.c new revision: 3.202; previous revision: 3.201 done Checking in jsiter.c; /cvsroot/mozilla/js/src/jsiter.c,v <-- jsiter.c new revision: 3.64; previous revision: 3.63 done Checking in jsiter.h; /cvsroot/mozilla/js/src/jsiter.h,v <-- jsiter.h new revision: 3.21; previous revision: 3.20 done The patch was broken as I assumed that JSGenerator can live without corresponding object. That was a bad idea. When the object becomes unreachable, its finalizer frees JSGenerator. Thus to root JSGenerator one has to root the object.

JSGenerator.obj must live so the only removal that I missed in the original patch in JSGenerator.next.

Comment on attachment 272543 [details] [diff] [review] Removal of JSGenerator.next Sorry, I should have seen the problem. /be

I checked in the patch from comment 26: Checking in jsiter.h; /cvsroot/mozilla/js/src/jsiter.h,v <-- jsiter.h new revision: 3.22; previous revision: 3.21 done

cvsroot/mozilla/js/tests/js1_8/genexps/regress-347739.js,v <-- regress-347739.js initial revision: 1.1 /cvsroot/mozilla/js/tests/js1_8/genexps/regress-349331.js,v <-- regress-349331.js initial revision: 1.1 /cvsroot/mozilla/js/tests/js1_8/genexps/regress-349012-01.js,v <-- regress-349012-01.js initial revision: 1.1

(In reply to comment #29) verified fixed 1.9.0 linux/mac*/windows. Note known trunk failures due to change in behavior on trunk: js1_7/geniter/regress-347739.js generator_instance.close readonly and immune: 2 expected: Inside finally: 1 Inside finally: 2 actual: Inside finally: 2 reason: Expected value 'Inside finally: 1 Inside finally: 2 ', Actual value 'Inside finally: 2 ' js1_7/geniter/regress-349012-01.js closing a generator fails to report error if yield during close is ignored expected: Inner finally,Outer finally actual: reason: Expected value 'Inner finally,Outer finally', Actual value '' js1_7/geniter/regress-349331.js test GC-invoke close expected: true actual: false reason: Expected value 'true', Actual value 'false' js1_7/iterable/regress-340526-02.js Iterators: cross-referenced objects with close handler can delay close handler execution expected: 2 actual: 0 reason: Expected value '2', Actual value '0'