347739 - Making generator_instance.close readonly and permanent (immutable)

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

The current rules about generator_instance.close allows to skip finally blocks around yield in the generator body via a simple assignment to  generator_instance.close. This defeats the purpose of finally and is very strange feature for JavaScript where there is no way to cancel from scripts execution of finally blocks in any other case. Moreover, this "feature" is available only when generators are closed through GC since for-in loop ignores generator_instance.close property.

Thus I suggest to make generator_instance.close readonly and immune to prevent messing with finally blocks.

The following test case demonstrates the problematic behavior: 

function gen_test(test_index) 
{
	try {
		yield 1;
	} finally {
		print("Inside finally: "+test_index);
	}
}

var iter1 = gen_test(1);
iter1.next();
iter1.close = null;
iter1 = null;
gc();

var iter2 = gen_test(2);
for (i in iter2)
    iter2.close = null;

Currently it prints when run with js shell:

TypeError: object is not a function
before 9232, after 9232, break 08179000
Inside finally: 2

instead of expected

Inside finally: 1
before 9232, after 9232, break 08179000
Inside finally: 2

Comment 1

[Brendan Eich [:brendan]]

(In reply to comment #0)
> The current rules about generator_instance.close allows to skip finally blocks
> around yield in the generator body via a simple assignment to 
> generator_instance.close. This defeats the purpose of finally and is very
> strange feature for JavaScript where there is no way to cancel from scripts
> execution of finally blocks in any other case.

This is a bug, since Python 2.5 does not allow gen.close to be overridden:

>>> def count2():
...  yield 1
...  yield 2
... 
>>> g = count2()
>>> g.close = None
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: 'generator' object has only read-only attributes (assign to .close)

> Moreover, this "feature" is
> available only when generators are closed through GC since for-in loop ignores
> generator_instance.close property.

The for-in loop does not call any close method other than the internal "eager finalize" hook for native default iterator objects that are known to be created by the for-in loop itself:

js> function count(n){for(let i=0;i<n;i++)yield i}
js> g = count(10)
[object Generator]
js> g.close = function(){print('g.close')}
function () {
    print("g.close");
}
js> for (i in g)print(i)
0
1
2
3
4
5
6
7
8
9

Breakpoint 1, js_CloseNativeIterator (cx=0x500180, iterobj=0x1805508) at jsiter.c:348
348         if (!JS_InstanceOf(cx, iterobj, &js_IteratorClass, NULL))
(gdb) n
368     }
(gdb) 
js_Interpret (cx=0x500180, pc=0x503949 "???\030\016?`\a", result=0xbfffe82c) at jsinterp.c:6061
6061                    sp[-2] = JSVAL_NULL;
(gdb) c
Continuing.
js> g = null
js> gc()
g.close
before 9232, after 9232, break 01008000
js> 

Here is an example of a native default iterator being eagerly finalized:

js> o = {a:1, b:2, c:3}   
[object Object]
js> for (i in o) if (i == 'b') break 

Breakpoint 1, js_CloseNativeIterator (cx=0x500180, iterobj=0x18055d8) at jsiter.c:348
348         if (!JS_InstanceOf(cx, iterobj, &js_IteratorClass, NULL))
(gdb) n
355         flags = JSVAL_TO_INT(OBJ_GET_SLOT(cx, iterobj, JSSLOT_ITER_FLAGS));
(gdb) 
356         if (!(flags & JSITER_ENUMERATE))
(gdb) 
364         if (iterobj == cx->cachedIterObj)
(gdb) 
365             cx->cachedIterObj = NULL;
(gdb) 
367         js_CloseIteratorState(cx, iterobj);
(gdb) s
js_CloseIteratorState (cx=0x500180, iterobj=0x18055d8) at jsiter.c:90
90          JS_ASSERT(JS_InstanceOf(cx, iterobj, &js_IteratorClass, NULL));
(gdb) n
91          slots = iterobj->slots;
(gdb) 
94          state = slots[JSSLOT_ITER_STATE];
(gdb) 
95          if (JSVAL_IS_NULL(state))
(gdb) 
99          parent = slots[JSSLOT_PARENT];
(gdb) 
100         if (!JSVAL_IS_PRIMITIVE(parent)) {
(gdb) 
101             iterable = JSVAL_TO_OBJECT(parent);
(gdb) 
103             if ((JSVAL_TO_INT(slots[JSSLOT_ITER_FLAGS]) & JSITER_FOREACH) &&
(gdb) 
110                 OBJ_ENUMERATE(cx, iterable, JSENUMERATE_DESTROY, &state, NULL);
(gdb) 
112         slots[JSSLOT_ITER_STATE] = JSVAL_NULL;
(gdb) 
113     }

This optimization is safe because the native default iterator cannot escape from its internal reference on the operand stack, and so is known to be unreachable when the loop terminates.  Note that if the loop reaches the end of the object's properties, the OBJ_ENUMERATE protocol will self-destruct the native iterator.

So the *only* way that generators are closed is via GC.

> Thus I suggest to make generator_instance.close readonly and immune to prevent
> messing with finally blocks.

This is easy to do -- were you going to patch jsiter.c or should I do it?  Time is short for Firefox 2 beta 2.

/be

Comment 2

[Brendan Eich [:brendan]]

Attachment: fix

Of course, one can still do hacks such as g.__proto__ = null and then g.close = null, but that's a SpiderMonkey extension that should be restricted over time as ES4/JS2 comes along.

/be

Comment 3

[Igor Bukanov]

Comment on attachment 232574 [details] [diff] [review]
fix

It is trivial to make generators immune to generator.__proto__ changing hack if js_CloseGeneratorObject would simply call generator_close.

Comment 4

[Brendan Eich [:brendan]]

Attachment: fix with proto-mutation bullet-proofing

Since generator_close is a native and may depend on argv[-2] (it does, via generator_send, which it calls directly), and that means faking up argv or else building a proper caller stack, I elected to change just js_CloseGeneratorObject to bypass gen->obj's __proto__ chain.

Since __parent__ is not mutable, new object's __parent__ is its constructor's __parent__, and the cached class object for Generator is the anonymous class constructor used to create generator instances, this is unbreakable.  It still seems to me we will want to lock down __proto__ against writes in most cases, in the near future.  But this should do for now.  Igor, what do you think?

/be

Comment 5

[Igor Bukanov]

(In reply to comment #4)
> 
> Since __parent__ is not mutable, new object's __parent__ is its constructor's
> __parent__, and the cached class object for Generator is the anonymous class
> constructor used to create generator instances, this is unbreakable.  It still
> seems to me we will want to lock down __proto__ against writes in most cases,
> in the near future.  But this should do for now.  Igor, what do you think?

Mutable __proto__ was necessary to build test cases for some GC hazards. But are there any other useful cases of changing __proto__?

Comment 6

[Igor Bukanov]

Comment on attachment 232643 [details] [diff] [review]
fix with proto-mutation bullet-proofing

Now finally is inevitable ;)

Comment 7

[Brendan Eich [:brendan]]

(In reply to comment #5)
> (In reply to comment #4)
> > 
> > Since __parent__ is not mutable, new object's __parent__ is its constructor's
> > __parent__, and the cached class object for Generator is the anonymous class
> > constructor used to create generator instances, this is unbreakable.  It still
> > seems to me we will want to lock down __proto__ against writes in most cases,
> > in the near future.  But this should do for now.  Igor, what do you think?
> 
> Mutable __proto__ was necessary to build test cases for some GC hazards. But
> are there any other useful cases of changing __proto__?

The main use-case that seems valid is to express an object initialiser that overrides __proto__:

var obj = {__proto__: proto, p1: v1, ... pN: vN};

This should still be supported in the JS2 future, IMHO.

Fixed on trunk.

/be

Comment 8

[Bob Clary [:bc] (inactive)]

Attachment: js1_7/geniter/regress-347739.js

This gives in both before and after:
STATUS: generator_instance.close readonly and immune
FAILED!: [reported from test()] generator_instance.close readonly and immune: 2
FAILED!: [reported from test()] Expected value 'Inside finally: 1 Inside finally: 2 ', Actual value 'Inside finally: 2 '
FAILED!: [reported from test()] 

before does show the object is not a function error, while after does not.

Comment 9

[Bob Clary [:bc] (inactive)]

before="before patch", after="after patch"

Comment 10

[Igor Bukanov]

Comment on attachment 232683 [details]
js1_7/geniter/regress-347739.js

The test is right but the committed patch was wrong.

Comment 11

[Igor Bukanov]

Comment on attachment 232643 [details] [diff] [review]
fix with proto-mutation bullet-proofing

>+    if (!JS_GetMethodById(cx, proto, id, &obj, &fval))
>         return JS_FALSE;

This is wrong as it sets obj to proto! So with the patch applied no finally handlers is going to be executed. I should have seen this before rubber-stamping the approval.

The right way to do it is just 
    if (!JS_GetMethodById(cx, proto, id, &proto, &fval))
         return JS_FALSE;

Comment 12

[Igor Bukanov]

Reopening per comment 11

Comment 13

[Igor Bukanov]

Attachment: Fixes for the test suite

This is fixes test cases where I relied on the option to change generatorInstance.close to get shorter test cases. This patch fixes the test cases to use finally block around yield. Plus the test iterable/regress-340526-02.js did  not test what it was supposed to do and this should address it.

Comment 14

[Bob Clary [:bc] (inactive)]

quick test on trunk windows from this morning:

340526 FAILED!: Expected value '2', Actual value '0'
341499 No Assert
341500 Pass (test is named wrong with wrong bug number)
341815 Pass

Comment 15

[Bob Clary [:bc] (inactive)]

Comment on attachment 232690 [details] [diff] [review]
Fixes for the test suite

this is fine as far as the format goes, but I'm not qualified to review what it is really testing.

Comment 16

[Bob Clary [:bc] (inactive)]

committed attachment 232690 [details] [diff] [review]

Checking in regress-340526-02.js;
/cvsroot/mozilla/js/tests/js1_7/iterable/regress-340526-02.js,v  <--  regress-340526-02.js
new revision: 1.2; previous revision: 1.1
done
Checking in regress-341499.js;
/cvsroot/mozilla/js/tests/js1_7/iterable/regress-341499.js,v  <--  regress-341499.js
new revision: 1.2; previous revision: 1.1
done
Checking in regress-341500.js;
/cvsroot/mozilla/js/tests/js1_7/iterable/regress-341500.js,v  <--  regress-341500.js
new revision: 1.2; previous revision: 1.1
done
Checking in regress-341815.js;
/cvsroot/mozilla/js/tests/js1_7/iterable/regress-341815.js,v  <--  regress-341815.js
new revision: 1.3; previous revision: 1.2
done

renamed test for bug 341510

Checking in regress-341510.js;
/cvsroot/mozilla/js/tests/js1_7/iterable/regress-341510.js,v  <--  regress-341510.js
initial revision: 1.1

Checking in regress-347739.js;
/cvsroot/mozilla/js/tests/js1_7/geniter/regress-347739.js,v  <--  regress-347739.js
initial revision: 1.1

Comment 17

[Igor Bukanov]

Attachment: More test suite fixes

The last test suite missed couple or files, so here is the rest of changes to use
try {
  yield ..
} finally {
    close_phase_code;
}

instead of assigning to generatorIterator.close.

Comment 18

[Bob Clary [:bc] (inactive)]

Checking in js1_7/GC/regress-341675.js;
/cvsroot/mozilla/js/tests/js1_7/GC/regress-341675.js,v  <--  regress-341675.js
new revision: 1.2; previous revision: 1.1
done
Checking in js1_7/iterable/regress-341821.js;
/cvsroot/mozilla/js/tests/js1_7/iterable/regress-341821.js,v  <--  regress-341821.js
new revision: 1.2; previous revision: 1.1
done

Comment 19

[Brendan Eich [:brendan]]

Attachment: followup fix

D'oh.  I too should have seen this one -- sorry.

/be

Comment 20

[Brendan Eich [:brendan]]

Attachment: consolidated patch for 1.8 branch

This is another small patch, built on solid primitives, where we have testsuite coverage (thanks to bc and igor).  Baking on the trunk is not as important as code review and testsuite coverage.  I think it can go into the 1.8 branch quickly.

/be

Comment 21

[Brendan Eich [:brendan]]

Fixed on trunk, for real.

/be

Comment 22

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 232772 [details] [diff] [review]
consolidated patch for 1.8 branch

a=drivers, please land on the branch.

Comment 23

[Brendan Eich [:brendan]]

Fixed on the 1.8 branch too.

/be

Comment 24

[Bob Clary [:bc] (inactive)]

testcase crashes with stack in Bug 348027 for the trunk debug browser|shell windows xp. I don't have 1.8 results for 20060809 as I screwed up the tests but I assume we have the same problem there.

Comment 25

[Bob Clary [:bc] (inactive)]

verified fixed 1.8, 1.9 20060818 windows/mac*/linux

Comment 26

[Bob Clary [:bc] (inactive)]

js1_8 version to reflect bug 380469

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