349012 - closing a generator fails to report error if GeneratorExit ignored

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Igor Bukanov]

The current generator implementation permits yield inside finally blocks. But this allows to skip outer finally blocks defeating the purpose of finally. Foe example, consider the following case for JS shell:

function gen()
{
        try {
                try {
                        yield 1;
                } finally {
                        print("Inner finally");
                        yield 2;
                }
        } finally {
                print("Outer finally");
        }
}

var iter = gen();
iter.next();
iter = null;
gc();

Currently it does not print "Outer finally", just "Inner finally" since yield during execution of the close hook invoked by GC quits the function. Given this I suggest to disallow using yield keyword inside finally blocks.

Comment 1

[Igor Bukanov]

(In reply to comment #0)
> Currently it does not print "Outer finally", just "Inner finally" since yield
> during execution of the close hook invoked by GC quits the function. Given this
> I suggest to disallow using yield keyword inside finally blocks.

An alternative solution would be to state that yield during closing just rethrow the original exception, but disallowing such questionable feature would be a better solution IMO.

Comment 2

[Igor Bukanov]

Note that Python does not have such problem since while allows yield inside finally, it does not allow yield inside try, so a translation of the above test into Python gives a syntax error:  

~> cat ~/s/x.py
def gen():
        try:
                try:
                        yield 1;
                finally:
                        print("Inner");
                        yield 2;
        finally:
                print("Outer");


~> python ~/s/x.py
  File "/home/igor/s/x.py", line 4
    yield 1;
SyntaxError: 'yield' not allowed in a 'try' block with a 'finally' clause

Comment 3

[Igor Bukanov]

(In reply to comment #2)
> ~> python ~/s/x.py
>   File "/home/igor/s/x.py", line 4
>     yield 1;
> SyntaxError: 'yield' not allowed in a 'try' block with a 'finally' clause
> 

I.e Python folks decided to take an easy way to implement generators without any extra GC support... Why it is so essential for JS?   

Comment 4

[Brendan Eich [:brendan]]

Igor, you are using Python 2.4.  Try Python 2.5, which is the version on which ES4 generators are based, and the "most modern" version:

$ cat x.py
def gen():
        try:
                try:
                        yield 1
                finally:
                        print("Inner")
                        yield 2
        finally:
                print("Outer")

x = gen()
print(x.next())
print(x.next())
print(x.next())

$ !py
python2.5 x.py
1
Inner
2
Outer
Traceback (most recent call last):
  File "x.py", line 14, in <module>
    print(x.next())
StopIteration

Here is the equivalent JS test and result:

$ cat x.js
function gen() {
        try {
                try {
                        yield 1
                } finally {
                        print("Inner")
                        yield 2
                }
        } finally {
                print("Outer")
        }
}

x = gen()
print(x.next())
print(x.next())
print(x.next())

h-231:~/Hacking/trunk/mozilla/js/src brendaneich$ ./Darwin_DBG.OBJ/js x.js
1
Inner
2
Outer
uncaught exception: [object StopIteration]

What is the bug?

/be

Comment 5

[Igor Bukanov]

(In reply to comment #4)
> 
> What is the bug?

The bug happens when GC executes the close handler as in the orignal example which I repeat here:

function gen()
{
        try {
                try {
                        yield 1;
                } finally {
                        print("Inner finally");
                        yield 2;
                }
        } finally {
                print("Outer finally");
        }
}

var iter = gen();
iter.next();
iter = null;
gc();


When executed it just prints:

Inner finally
before 9232, after 9232, break 0817a000

I.e. it does not execute the outer finally.

Comment 6

[Brendan Eich [:brendan]]

The bug is that generator_close should ignore generator_send's return value when it uses generator_send to throw GeneratorExit at the generator-iterator.  Right now, a yield from the generator-iterator will cause generator_close to return ok right away.

In the course of fixing this, I improved the diagnostic, which motivated adding the missing Generator toSource and toString methods.

Test results vs. Python 2.5:

$ cat x2.py
def gen():
        try:
                try:
                        yield 1
                finally:
                        print("Inner")
                        yield 2
        finally:
                print("Outer")

x = gen()
print(x.next())
x = None

$ python2.5 x2.py
1
Inner
Exception exceptions.RuntimeError: 'generator ignored GeneratorExit' in <generator object at 0x649e0> ignored
$
$ cat x2.js
function gen() {
        try {
                try {
                        yield 1
                } finally {
                        print("Inner")
                        yield 2
                }
        } finally {
                print("Outer")
        }
}

x = gen()
print(x.next())
x = null
gc()

$ ./Darwin_DBG.OBJ/js x2.js
1
Inner
TypeError: generator function gen() {try {try {yield 1;} finally {print("Inner");yield 2;}} finally {print("Outer");}} ignored GeneratorExit
before 9232, after 9232, break 01205000

There's no further bug -- you can't make a generator-iterator run after it has been closed, even if it yielded from close.  Finally clauses may fail to run in any system that does not allow functions to deny service, but in this case there is simply no further protocol or explicit call to resume the closed generator (and if there were, it would properly throw StopIteration).

Patch next.

/be

Comment 7

[Brendan Eich [:brendan]]

Attachment: fix

Patch as described, plus a compression of JS_InstanceOf/JS_GetPrivate in generator_send into JS_GetInstancePrivate.

/be

Comment 8

[Mike Schroepfer]

Putting on the 1.8.1/FF2 radar...

Comment 9

[Igor Bukanov]

(In reply to comment #6)
> There's no further bug -- you can't make a generator-iterator run after it has
> been closed, even if it yielded from close.  Finally clauses may fail to run in
> any system that does not allow functions to deny service, but in this case
> there is simply no further protocol or explicit call to resume the closed
> generator (and if there were, it would properly throw StopIteration).

I think Python got it wrong with this explicit language construction to skip the finally block. When the generator executes yield during the close, that yield instead of yielding should just throw an exception, which would allow to execute the outer finally blocks in a clean way.

Comment 10

[Igor Bukanov]

Consider another test case about bad finally behavior:

function gen()
{
        try {
                throw 1;
        } finally {
                yield 2;
        }
}

var iter = gen();
var value = iter.next();
print(value);

Even with the patch applied it just prints 2 without any thing of the thrown exception. I think in this is another case when yield should yield. I.e. when  finally block executes under thrown exception and finally call yields, that should throw an exception.

Comment 11

[Brendan Eich [:brendan]]

It depends on whether you think finally should always run (but that can't be, to prevent DOSes we will break this rule anyway), or incomparably, whether yield is like return.  Yield is really like return in Python.  Neither can throw intrinsically (their operands can throw during evaluation of course).

For the moment I would like get your review stamp to get the patch in, and match Python 2.5 behavior.

I'll raise this issue in the ECMA TG1 wiki.

/be

Comment 12

[Igor Bukanov]

Python 2.5 also swallows exceptions silently: 

~> cat ~/s/x.py
def gen():
        try:
                raise NameError, 'HiThere';
                y();
        finally:
                print "Inside finally";
                yield "yield from finally";

x = gen();
y = x.next();
print "y="+y;
x=None;
~> ~/s/Python-2.5c1/python ~/s/x.py
Inside finally
y=yield from finally


This is really must be a bug there.

Comment 13

[Igor Bukanov]

(In reply to comment #11)
> It depends on whether you think finally should always run (but that can't be,
> to prevent DOSes we will break this rule anyway), 

The rule was always broken long time ago since a branch callback can stop any script. But this is really an implementation detail and not a language feature. Currently there is no way to avoid finally for scripts and IMO JS should continue to hold this.

> or incomparably, whether
> yield is like return.  Yield is really like return in Python.  Neither can
> throw intrinsically (their operands can throw during evaluation of course).

But finally of the try block with return is executed and can throw even in Python, so return *can* throw. Which means that yield is not like return there. 

> 
> For the moment I would like get your review stamp to get the patch in, and
> match Python 2.5 behavior.

But do we want to copy bugs from not yet release Python version to JS?

Comment 14

[Brendan Eich [:brendan]]

You're right -- try-finally{return} leaves the exception pending on return, but yield quashes it.

We emulate faithfully:

$ ./Darwin_DBG.OBJ/js x3.js
Inside finally
y=yield from finally
before 9232, after 9232, break 01205000
h-231:~/Hacking/trunk/mozilla/js/src brendaneich$ cat x3.js
function gen() {
        try {
                throw 'HiThere'
                y()
        } finally {
                print("Inside finally")
                yield "yield from finally"
        }
}

x = gen()
y = x.next()
print("y="+y)
x = null
gc()

$ ./Darwin_DBG.OBJ/js x3.js
Inside finally
y=yield from finally
before 9232, after 9232, break 01205000

I've pinged Robert Sayre, who may be able to get Bob Ippolito to comment here.

/be

Comment 15

[Igor Bukanov]

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

+ for cleanup, not ignoring finally!

Comment 16

[Igor Bukanov]

Here is an extract from Enhanced Generators for Python proposal.
http://www.python.org/dev/peps/pep-0342/ :

4. Add a close() method for generator-iterators, which raises
       GeneratorExit at the point where the generator was paused.  If
       the generator then raises StopIteration (by exiting normally, or
       due to already being closed) or GeneratorExit (by not catching
       the exception), close() returns to its caller.  If the generator
       yields a value, a RuntimeError is raised.  If the generator
       raises any other exception, it is propagated to the caller.
       close() does nothing if the generator has already exited due to
       an exception or normal exit.

My reading of this is that it states that the yield statement just throws during the close phase so pending finally should be executed and the prereleased versions of Python 2.5 have a bug or not yet implemented feature. 

Comment 17

[Brendan Eich [:brendan]]

Patch misnamed "fix" is in on trunk.

Leaving this bug open -- losing exceptions *and* failing to run finally blocks *does* look like a bug, especially since Python 2.4 disallowed throw in try, precisely because of the finally-always-runs violation that throw in try would have created in the absence of the PEP 342 / Python 2.5 close protocol.

I'll patch yield to throw if closing next.

/be

Comment 18

[Bob Ippolito]

As speculated, I'm pretty sure this is indeed a Python 2.5c1 bug. I've reported it: http://python.org/sf/1542308

The right behavior should be to print "Inner finally", "Outer finally" and then the GC would eat a RuntimeError.

Additionally, if the code is changed to:
var iter = gen()
iter.next();
iter.close();

then it should print "Inner finally", "Outer finally" and then throw a RuntimeError to the caller of close().

Python 2.5c1 does not handle either case correctly at this point.

Comment 19

[Brendan Eich [:brendan]]

Attachment: Fix the bug cloned from Python 2.5 beta behavior

This is pretty straightforward.  Igor, I noticed that there was no idempotency guard in generator_close, which means an explicit close call can be followed by an implicit one from the GC.  Was this defended against elsewhere?  In re-factoring the common send/close subroutine, I defended in close using the revised meaning of JSGEN_CLOSED.

The new JSGEN_DONE and JSGEN_CLOSING states are used with JSGEN_CLOSED to fix the bug copied from Python 2.5 beta, by making JSOP_YIELD throw if the generator state is JSGEN_CLOSING.

Let me know if anything looks wrong.  A diff -w is next, for slightly easier reviewing.  Thanks to Igor for finding this bug.

/be

Comment 20

[Brendan Eich [:brendan]]

Attachment: diff -w version of last attachment

Comment 21

[Igor Bukanov]

Comment on attachment 234340 [details] [diff] [review]
Fix the bug cloned from Python 2.5 beta behavior

>     if (!ok) {
>         if (cx->throwing)
>-            gen->state = JSGEN_CLOSED;
>+            gen->state = JSGEN_DONE;
>         return JS_FALSE;
>     }
> 
>     if (!(gen->frame.flags & JSFRAME_YIELDING)) {
>         /* Returned, explicitly or by falling off the end. */
>-        gen->state = JSGEN_CLOSED;
>+        gen->state = JSGEN_DONE;
>         return js_ThrowStopIteration(cx, obj);
>     }

Is JSGEN_DONE is necessary? 

When the generator exits via a return or exception, there is no finally blocks left to run on close so effectively JSGEN_DONE is JSGEN_CLOSE. But with JSGEN_DONE the close method does send GeneratorExit  just to immediately catch and ignore StopIteration. In fact GC should not even schedule closed (or "done") generator objects and finalize them immediately when they become unreachable.

Comment 22

[Igor Bukanov]

Yet another problem with generators. What is supposed to happen when the generator calls .next/.send/.close on itself when running?

The following test currently asserts:

~> cat ~/s/x.js
var iter;

var N = 10;

function gen() {
        if (N == 0) {
                yield "something";
        } else {
                --N;
                iter.next();
        }
}

iter = gen();
var i = iter.next();
print("i="+i);

~> js ~/s/x.js
Assertion failure: JSVAL_IS_BOOLEAN(thisv), at jsinterp.c:1203
Trace/breakpoint trap

The send version exits with the infinite recursion message:

~> cat ~/s/x.js
var iter;

var N = 10;

function gen() {
        if (N == 0) {
                yield "something";
        } else {
                --N;
                iter.send(1);
        }
}

iter = gen();
var i = iter.next();
print("i="+i);

~> js ~/s/x.js
/home/igor/s/x.js:11: InternalError: too much recursion

A close example ignores close:
~> cat ~/s/x.js
var iter;
function gen() {
        iter.close();
        yield 1;
}

iter = gen();
iter.next();
print("Done");
~> js ~/s/x.js
Done

I think all this examples should throw an exception about bad generator usage.

Comment 23

[Igor Bukanov]

(In reply to comment #22)
> ..examples should throw an exception about bad generator usage.

<rant>
I wish iterator protocol would never be exposed to scripts and the only way to work iterators would be through for-in loop. But it is too late to close Pandora box and the only hope that the damage would be concealed before FF2.
</rant>  

Comment 24

[Brendan Eich [:brendan]]

(In reply to comment #22)
> Yet another problem with generators. What is supposed to happen when the
> generator calls .next/.send/.close on itself when running?

$ cat x5.py
iter = None

N = 10

def gen():
        if N == 0:
                yield "something"
        else:
                --N
                iter.next()

iter = gen()
i = iter.next()
print("i="+i)

$ python2.5 x5.py
Traceback (most recent call last):
  File "x5.py", line 13, in <module>
    i = iter.next()
  File "x5.py", line 10, in gen
    iter.next()
ValueError: generator already executing

Fixing in next patch.

/be

Comment 25

[Brendan Eich [:brendan]]

Attachment: Last patch, plus throw on nesting generator execution

Comment 26

[Brendan Eich [:brendan]]

Attachment: diff -w version of last attachment

For easier reviewing.

/be

Comment 27

[Brendan Eich [:brendan]]

Igor: rant aside, the problem child is not iterators but generators.  Without your help we wouldn't have made it this far.  Thanks again for that.

There are strong benefits to pushing JS at this pace, as well as costs.  Will it all work out as a net positive for JS developers?  I think so -- I believe that the state space is not so large that we won't have it covered by the time Firefox 2 ships.  More testing will tell.

/be

Comment 28

[Igor Bukanov]

Comment on attachment 234452 [details] [diff] [review]
Last patch, plus throw on nesting generator execution

>     /* Store the argument to send as the result of the yield expression. */
>     gen->frame.sp[-1] = (argc != 0) ? argv[0] : JSVAL_VOID;
>+    gen->state = JSGEN_RUNNING;
>     ok = js_Interpret(cx, gen->frame.pc, &junk);
>+    gen->state = JSGEN_OPEN;
>     cx->fp = fp;
>     cx->stackPool.current = arena;
> 
>     if (!ok) {
>         if (cx->throwing)
>             gen->state = JSGEN_CLOSED;
>         return JS_FALSE;
>     }

I think this should be
     if (!ok) {
         JS_ASSERT(!(gen->frame.flags & JSFRAME_YIELDING));
         gen->state = JSGEN_CLOSED;
         return JS_FALSE;
     }

since quit-ASAP signal like a cancellation form the branch callback by definition skips any finally. So generator should become closed in this case as well.

Otherwise it is OK.

Comment 29

[Brendan Eich [:brendan]]

Attachment: Last patch with Igor's suggested change

Also, with the JSMSG_BAD_GENERATOR_YIELD error message from the JSOP_YIELD case in the interpreter now citing the string result of js_DecompileValueGenerator, not js_ValueToPrintableSource.  Even though js_DVG won't produce anything other than js_ValueToSource when the value is an argument (fodder for another bug), it is the right internal API to use for diagnostics.

I'll check this in soon.

/be

Comment 30

[Brendan Eich [:brendan]]

Attachment: diff -w version of last attachment

For reference.

/be

Comment 31

[Brendan Eich [:brendan]]

Fixed on trunk.  Preparing all-in-one branch patch.

/be

Comment 32

[Brendan Eich [:brendan]]

Attachment: all-in-one 1.8 branch patch created by cvs up -j

Comment 33

[Igor Bukanov]

(In reply to comment #27)
> the problem child is not iterators but generators.

Indeed, there would be no problems if the only way to use generatots would be through for-in loop so scripts would have no access to iterating object corresponding to the generator. 

> There are strong benefits to pushing JS at this pace, as well as costs.  Will
> it all work out as a net positive for JS developers?  I think so -- I believe
> that the state space is not so large that we won't have it covered by the time
> Firefox 2 ships.  More testing will tell.

As I can see among new JS1.7 features that option to manipulate explicitly instances of generator() leaded to majority of time-consuming problems. Moreover, the problems are not yet over.

For example, the transitions between generator states are not thread-safe and proper locking is not-trivial in presense of untrusted code. Another problem is that close hooks from untrusted code can run at arbitrary moments during execution of trusted scripts and surely there are security implications of that.

Surely everything can be fixed, but for me all these troubles indicates a broken design.

Comment 34

[Brendan Eich [:brendan]]

(In reply to comment #33)
> (In reply to comment #27)
> > the problem child is not iterators but generators.
> 
> Indeed, there would be no problems if the only way to use generatots would be
> through for-in loop so scripts would have no access to iterating object
> corresponding to the generator.

OTOH generators give relief from twisty callbacks and manual state management across callbacks.
 
> As I can see among new JS1.7 features that option to manipulate explicitly
> instances of generator() leaded to majority of time-consuming problems.

All the features have cost me a lot of time; destructuring assignment was a big pile of patches too, but conceptually simple syntactic sugar.

> Moreover, the problems are not yet over.

Not for Firefox 2?  Let's see:

> For example, the transitions between generator states are not thread-safe and
> proper locking is not-trivial in presense of untrusted code.

This is not a problem for Firefox 2 in my view.

> Another problem is
> that close hooks from untrusted code can run at arbitrary moments during
> execution of trusted scripts and surely there are security implications of
> that.

We have general mechanism to impute trust based on a stack walk; so long as the untrusted code can't impersonate the trusted code, we are safe from XSS attacks. This needs to be tested harder, of course -- I will engage our white hats.

> Surely everything can be fixed, but for me all these troubles indicates a
> broken design.

You could say the lack of thread safety is a design flaw, but I don't see the security worries as proof of design badness.

On the subject of threads: threads are never going to be reflected into the core JS standard, because threads break abstraction.  Exposing threads in the usual style exposes deadlocks, race conditions, and all the usual hazards and costs. We should talk more in a separate bug; I'll file it and cite it here in a bit.

You may have noticed that the E4X code is also not thread-safe.  This too is a feature, as far as I am concerned.  If it demonstrates broken design to you, then I claim the DOM is likewise a broken design.  So is Gecko (and to the extent that Gecko can't fly modal dialogs for separate windows and have them dismiss in arbitrary order, I agree -- but fixing that doesn't require threads).

Threads need motivation -- we shouldn't just make every abstraction thread-safe at all cost.  Better to make single-threaded abstractions that are isolated.

/be

Comment 35

[Brendan Eich [:brendan]]

I filed bug 349263 on thread-safety for iterators and generators.

/be

Comment 36

[Igor Bukanov]

(In reply to comment #34)
> > Another problem is
> > that close hooks from untrusted code can run at arbitrary moments during
> > execution of trusted scripts and surely there are security implications of
> > that.
> 
> We have general mechanism to impute trust based on a stack walk; so long as the
> untrusted code can't impersonate the trusted code, we are safe from XSS
> attacks. This needs to be tested harder, of course -- I will engage our white
> hats.

A trivial example to demonstrate that something has to be done:

<html><body><script>

function gen()
{
  function f()
  {
    prompt("Enter your password:");
  }
  try {
    yield 1;
  } finally {
    setTimeout(f, 5000);
  }
}

var iter = gen();
iter.next();
</script></body></html>

With this example I got a prompt for password on a page I went back from the page with the script. Again, this demonstrates that API to control close hooks from the browser must be implemented brining yet another complexity. 

Comment 37

[Brendan Eich [:brendan]]

gen->state manipulation changes broke the original testcase:

function gen() {
        try {
                try {
                        yield 1
                } finally {
                        print("Inner")
                        yield 2
                }
        } finally {
                print("Outer")
        }
}

x = gen()
print(x.next())
print(x.close())

Also, something is rotten still with this tescase:

var iter;

var N = 10;

function gen() {
        if (N == 0) {
                yield "something";
        } else {
                --N;
                iter.next();
        }
}

iter = gen();
var i = iter.next();
print("i="+i);

/be

Comment 38

[Brendan Eich [:brendan]]

Attachment: fix generator state coding and testing

This is much better.  The composeable state bits allowed inlining of ReportNestingGenerator.

/be

Comment 39

[Brendan Eich [:brendan]]

Attachment: diff -w version of last attachment

Comment 40

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 234529 [details] [diff] [review]
fix generator state coding and testing

Sorry, I should have seen this before.

Comment 41

[Brendan Eich [:brendan]]

(In reply to comment #36)
> Again, this demonstrates that API to control close hooks
> from the browser must be implemented brining yet another complexity.

That complexity is already there: we have to clear window scope, timeouts and intervals, and (in Gecko) watchpoints on page transitions.  I keep forgetting that this bug hasn't been fixed yet.  Did you ever file it?  I know we talked about it in another bug.

/be

Comment 42

[Igor Bukanov]

Comment on attachment 234529 [details] [diff] [review]
fix generator state coding and testing

I should have seen this state game instead of spending time on <rant> ;)

>     /* Throw GeneratorExit at the generator and ignore the returned status. */
>     JS_SetPendingException(cx, genexit);
>-    gen->state = JSGEN_CLOSING;
>+    gen->state |= JSGEN_CLOSING;
>     generator_send_sub(cx, obj, gen, 0, argv, rval);
>     gen->state = JSGEN_CLOSED;

I did not notice it before but ignoring generator_send_sub result here means that the branch callback request to cancel the execution would be ignored. It is OK otherwise.

Comment 43

[Brendan Eich [:brendan]]

Attachment: fix per Igor's last comment

Comment 44

[Brendan Eich [:brendan]]

Attachment: diff -w version of last attachment

Comment 45

[Brendan Eich [:brendan]]

Comment on attachment 234533 [details] [diff] [review]
fix per Igor's last comment

It's been a long week for everyone.  Take your time on the reviews. :-)

/be

Comment 46

[Igor Bukanov]

Comment on attachment 234533 [details] [diff] [review]
fix per Igor's last comment

:)

Comment 47

[Igor Bukanov]

(In reply to comment #41)
 keep forgetting
> that this bug hasn't been fixed yet.  Did you ever file it?  I know we talked
> about it in another bug.

I was about to file a bug about preventing DOS through close hooks when I discovered the problems reported in this bug. But now we got bug 349272. 

Comment 48

[Brendan Eich [:brendan]]

Fixed, I hope.  Branch patch in a bit.

/be

Comment 49

[Brendan Eich [:brendan]]

Attachment: combined 1.8 branch patch

Comment 50

[Brendan Eich [:brendan]]

Attachment: combined 1.8 branch patch

Argh, didn't cp before attaching.

/be

Comment 51

[Bob Clary [:bc] (inactive)]

RCS file: /cvsroot/mozilla/js/tests/js1_7/geniter/regress-349012-01.js,v
done
Checking in regress-349012-01.js;
/cvsroot/mozilla/js/tests/js1_7/geniter/regress-349012-01.js,v  <--  regress-349012-01.js
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/js/tests/js1_7/geniter/regress-349012-02.js,v
done
Checking in regress-349012-02.js;
/cvsroot/mozilla/js/tests/js1_7/geniter/regress-349012-02.js,v  <--  regress-349012-02.js
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/js/tests/js1_7/geniter/regress-349012-03.js,v
done
Checking in regress-349012-03.js;
/cvsroot/mozilla/js/tests/js1_7/geniter/regress-349012-03.js,v  <--  regress-349012-03.js
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/js/tests/js1_7/geniter/regress-349012-04.js,v
done
Checking in regress-349012-04.js;
/cvsroot/mozilla/js/tests/js1_7/geniter/regress-349012-04.js,v  <--  regress-349012-04.js
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/js/tests/js1_7/geniter/regress-349012-05.js,v
done
Checking in regress-349012-05.js;
/cvsroot/mozilla/js/tests/js1_7/geniter/regress-349012-05.js,v  <--  regress-349012-05.js
initial revision: 1.1
done

Comment 52

[Mike Schroepfer]

Comment on attachment 234556 [details] [diff] [review]
combined 1.8 branch patch

a=schrep for drivers - approving all [181approval pending] bugs now that tree is open.

Comment 53

[Bob Clary [:bc] (inactive)]

Igor, is there anyway to catch the GeneratorExit ignored by generator function error, or did I misname this test?

Comment 54

[Brendan Eich [:brendan]]

(In reply to comment #18)
> As speculated, I'm pretty sure this is indeed a Python 2.5c1 bug. I've reported
> it: http://python.org/sf/1542308

Does anyone reading the python.org tracker entry linked above think that PEP 342 (http://www.python.org/dev/peps/pep-0342/) is unambiguous?  Its "Specification Summary" says:

    4. Add a close() method for generator-iterators, which raises
       GeneratorExit at the point where the generator was paused.  If
       the generator then raises StopIteration (by exiting normally, or
       due to already being closed) or GeneratorExit (by not catching
       the exception), close() returns to its caller.  If the generator
       yields a value, a RuntimeError is raised.

The question is, *when* is a RuntimeError raised if the generator yields a value during close?  The implementation in Python 2.5c1 follows the Python code given later on, under "New generator method: close()", and raises from close after the yield has transferred control back to close, without running any finallys that should run for the block containing that yield.

The alternative reading of the last sentence from item 4 is that yield itself throws when executing from close.  That's what we have now implemented in JS.

To recap, with an implicit close from the runtime, there's no "Outer":

$ cat x2.py
def gen():
        try:
                try:
                        yield 1
                finally:
                        print("Inner")
                        yield 2
        finally:
                print("Outer")

x = gen()
print(x.next())
x = None

$ python2.5 x2.py
1
Inner
Exception exceptions.RuntimeError: 'generator ignored GeneratorExit' in <generator object at 0x649e0> ignored

In contrast, an explicit close is aborted by the RuntimeError after the generator was resumed, printed "Inner", and yielded 2.  Later, from the ref-counting system or GC, there's a second close, which does print "Outer".

Sure, the generator is broken, but stuff happens.  The question is, which reading of "If the generator yields a value, a RuntimeError is raised" produces more useful and consistent results.  Getting "Outer" both times (implicit and explicit close) seems both more useful and more consistent.

Talking about iloops is a digression (browsers have to police iloops everywhere; there are many ways to write iloops, with or without generators).  Talking about IronPython, Jython, etc., without demonstrating how an alternative that raises a RuntimeError from yield itself is hard to implement in other VMs does not prove anything.

It would be good to get to the bottom of this before we go further.  As things stand right now, JS is going to differ from Python on this point.

/be

Comment 55

[Igor Bukanov]

> As things
> stand right now, JS is going to differ from Python on this point.

See also bug 349331 where I suggest to remove GeneratorExit completely and instead to make generatorInstance.close() to be an equivalent of return after yield. This avoids all the mess about catching/not catching GeneratorExit and would lead to very easy to grasp behavior:

next:   resume after yield
throw: throw after yield
close:  return after yield  

I got an impression from Python mail lists that for 3.0 there are plans to use exceptions for control transfer like break so break can be caught and ignored. In view of such plans GeneratorExit makes sence, but I hope such plans would never come to JS.

Comment 56

[Brendan Eich [:brendan]]

(In reply to comment #55)
> I got an impression from Python mail lists that for 3.0 there are plans to use
> exceptions for control transfer like break so break can be caught and ignored.
> In view of such plans GeneratorExit makes sence, but I hope such plans would
> never come to JS.

No such plans!

/be

Comment 57

[Igor Bukanov]

(In reply to comment #54)
> The question is, *when* is a RuntimeError raised if the generator yields a
> value during close?  The implementation in Python 2.5c1 follows the Python code
> given later on, under "New generator method: close()", and raises from close
> after the yield has transferred control back to close, without running any
> finallys that should run for the block containing that yield.
> 
> The alternative reading of the last sentence from item 4 is that yield itself
> throws when executing from close.  That's what we have now implemented in JS.

Consider the following example for Python 2.5r1:

def gen():
  for i in range(5):
    try:
      print "BEFORE YIELD, i:",i
      yield i
      print "AFTER YIELD, i:", i
    except:
      print "Unexpected exception, i:", i
    finally:
      print "FINALLY"


iter = gen()
for i in iter:
  print i
  try:
    iter.close()
  except Exception, e:
    print "Exception on first close: ", e
  try:
    iter.close()
  except Exception, e:
    print "Exception on the second close: ", e
  break

print "AFTER LOOP"
iter=None
print "END"

It prints:

BEFORE YIELD, i: 0
0
Unexpected exception, i: 0
FINALLY
BEFORE YIELD, i: 1
Exception on first close:  generator ignored GeneratorExit
Unexpected exception, i: 1
FINALLY
BEFORE YIELD, i: 2
Exception on the second close:  generator ignored GeneratorExit
AFTER LOOP
Unexpected exception, i: 2
FINALLY
BEFORE YIELD, i: 3
Exception exceptions.RuntimeError: 'generator ignored GeneratorExit' in <generator object at 0xb7d18a0c> ignored
END

My interpretation of this is that the exception is raised after yield transfers control to the caller. Since the generator yielding, Python does not mark it as closed. So when all reference to the generator gone, Python tries to close it the final time. That yields again, so close throws an exception which is ignored by Python GC. What happens to the pending finally blocks is unclear. 

For me it sounds like a bug, not feature. 

Comment 58

[Igor Bukanov]

(In reply to comment #56)
> (In reply to comment #55)
> > I got an impression from Python mail lists that for 3.0 there are plans to use
> > exceptions for control transfer like break so break can be caught and ignored.
> > In view of such plans GeneratorExit makes sence, but I hope such plans would
> > never come to JS.
> 
> No such plans!

Here is some defense for Python. It is possible to ignore break even with JS:

for (var i = 0; i != 5; ++i) {
    try {
        try {
            break;
        } finally {
            throw 1;
        }
    } catch (e) {
    }
    print("i="+i);
}

So using exceptions for control transfer is not that crazy. On the other hand from
http://www.google.no/search?q=GeneratorExit+site:mail.python.org&hl=no&hs=pPo&lr=&client=firefox-a&rls=org.mozilla:en-US:unofficial&start=10&sa=N
I got an impression that those control transfer exceptions are not going to be easy-to-caught with a generic catch.  

Comment 59

[Brendan Eich [:brendan]]

Summary so anyone who wants to skip all the long comments can catch up:

- PEP 342 prose combined with code for the close method specify that RuntimeError for yield from a generator resumed by close must be raised by close after the generator has yielded control.  So any finally clause in the generator that has not run, will not run, if the generator yields during close.

- This is considered the best defense against a broken generator by Python gurus.

- JS 1.7 throws from yield bytecode if called from close, keeping the finally promise.  This is considered more useful and consistent even in the face of broken generator by JS gurus.

- Speaking from ECMA TG1 experience, I predict no evolution of JS will reflect break, continue, return, or yield as directly catchable exceptions.

I'll comment in bug 349331 now.

/be

Comment 60

[Brendan Eich [:brendan]]

(In reply to comment #59)
> Summary so anyone who wants to skip all the long comments can catch up:
> 
> - PEP 342 prose combined with code for the close method specify that
> RuntimeError for yield from a generator resumed by close must be raised by
> close after the generator has yielded control.  So any finally clause in the
> generator that has not run, will not run, if the generator yields during close.

Unless, of course, there's a second close attempt.  Which there will be, from the GC, if the first attempt was explicit.

The fact that this takes too many sentences with dependent clauses says to me that it's broken.  Where's the simplicity?

/be

Comment 61

[Brendan Eich [:brendan]]

Fixed on the 1.8 branch.

/be

Comment 62

[Bob Clary [:bc] (inactive)]

I still fail in js1_7/geniter/regress-349012-01.js with an uncaught exception TypeError: GeneratorExit ignored by generator function gen() ... Should I rename this test to regress-349012-01-n.js to show it expects to end with an uncaught exception?

All other tests 02-05 passed 1.8, 1.9 20060824 windows/mac*/linux

Comment 63

[Igor Bukanov]

(In reply to comment #62)
> I still fail in js1_7/geniter/regress-349012-01.js with an uncaught exception
> TypeError: GeneratorExit ignored by generator function gen() ... Should I
> rename this test to regress-349012-01-n.js to show it expects to end with an
> uncaught exception?

Note that GeneratorExit will be removed, bug 349331, so the tests should be updated at that point. 

Comment 64

[Bob Clary [:bc] (inactive)]

Ok, with a trunk winxp build from this afternoon (2006082714 ET) I am getting funky results on <http://test.bclary.com/tests/mozilla.org/js/js-test-driver-standards.html?test=js1_7/geniter/regress-349012-01.js;language=language;javascript>:

BUGNUMBER: 349012

STATUS: closing a generator fails to report error if GeneratorExit ignored
gc: [Exception... "Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [jsdIDebuggerService.GC]" nsresult: "0x80040111 (NS_ERROR_NOT_AVAILABLE)" location: "JS frame :: http://test.bclary.com/tests/mozilla.org/js/js1_7/browser.js :: gc :: line 187" data: no]
FAILED!: [reported from test()] yield from closing generator actual

Note that I could not catch this exception. If that is intended, I will change this test to negative -n.js.

Install venkman <https://addons.mozilla.org/firefox/216/> and run the test case:

BUGNUMBER: 349012

STATUS: closing a generator fails to report error if GeneratorExit ignored

It passed. 

But if you set a breakpoint in the gen() function, then single step through the remainder of the test, the test will fail with the actual variable not being set. If you continue on single stepping through the functions which report the error until you finish, then close venkman and then close the browser you crash with a bad vtable in an xpcom pointer. I did this twice: once with a deleted vtable pointer and one with trash. I'll attach the stacks.

Comment 65

[Bob Clary [:bc] (inactive)]

Attachment: funkytown stacks

Comment 66

[Igor Bukanov]

Attachment: Test suite changes to reflect bug 349331

Since bug 349331 eliminated GeneratorExit, generatorInstance.close() does not need to report that the the generator exit with anything but GeneratorExit hiding the original exception. Instead all the exceptions are passed to the caller. 

If the caller is an explicit generatorInstance.close(), then it is possible to catch the exception, but if the caller is the GC like in js1_7/geniter/regress-349012-01.js, then the exception is just reported to the embedding and ignored. Hence it is OK and is expected if browser or js shell prints about it.

Comment 67

[Bob Clary [:bc] (inactive)]

thanks. 

Checking in js1_7/geniter/regress-349012-01.js;
/cvsroot/mozilla/js/tests/js1_7/geniter/regress-349012-01.js,v  <--  regress-349012-01.js
new revision: 1.2; previous revision: 1.1
done
Checking in js1_7/geniter/regress-349012-05.js;
/cvsroot/mozilla/js/tests/js1_7/geniter/regress-349012-05.js,v  <--  regress-349012-05.js
new revision: 1.2; previous revision: 1.1
done

The crash when js debugging is troublesome though.

Comment 68

[Igor Bukanov]

(In reply to comment #64)
> Install venkman <https://addons.mozilla.org/firefox/216/> and run the test
> case:
> 
> BUGNUMBER: 349012
> 
> STATUS: closing a generator fails to report error if GeneratorExit ignored
> 
> It passed. 

My tests failed with:

gc: A script from "http://test.bclary.com" was denied UniversalXPConnect privileges.

There is no dialog asking for that with trunk build and adding 
user_pref("capability.principal.tests.id","http://test.bclary.com/tests/mozilla.org/js/js-test-driver-standards.html");
user_pref("capability.principal.tests.granted","UniversalXPConnect");

to user.js does not help either :(

Comment 69

[Bob Clary [:bc] (inactive)]

(In reply to comment #68)

you need user_pref("signed.applets.codebase_principal_support", true);

Comment 70

[Igor Bukanov]

(In reply to comment #69)
> (In reply to comment #68)
> 
> you need user_pref("signed.applets.codebase_principal_support", true);
> 

Thanks, I forgot about that preference!

Still the test from [1] does not run:

FAILED!: Error loading script

FAILED!: Error loading script
BUGNUMBER: 349012
STATUS: closing a generator fails to report error if GeneratorExit ignored

[1] http://test.bclary.com/tests/mozilla.org/js/js-test-driver-standards.html?test=js1_7/geniter/regress-349012-01.js;language=language;javascript



FAILED!: Error loading script

Comment 71

[Bob Clary [:bc] (inactive)]

sorry, I forgot the version goodness.

<http://test.bclary.com/tests/mozilla.org/js/js-test-driver-standards.html?test=js1_7/geniter/regress-349012-01.js;language=type;text/javascript;version=1.7>

Comment 72

[Igor Bukanov]

(In reply to comment #71)
> sorry, I forgot the version goodness.
> 
> <http://test.bclary.com/tests/mozilla.org/js/js-test-driver-standards.html?test=js1_7/geniter/regress-349012-01.js;language=type;text/javascript;version=1.7>
> 

Should it be:
http://test.bclary.com/tests/mozilla.org/js/js-test-driver-standards.html?test=js1_7/geniter/regress-349012-01.js;language=javascript;type=text/javascript;version=1.7
                                                                                                                                  

I.e. the parameters should be:
language=javascript;type=text/javascript;version=1.7
                                                                                                                                        ?
                                                                                                                                                                                                                                                                                                                                                                                    

Still the same:

FAILED!: Error loading script

FAILED!: Error loading script
BUGNUMBER: 349012
STATUS: closing a generator fails to report error if GeneratorExit ignored

Comment 73

[Igor Bukanov]

OK, I got the idea behind the way url's parameters are parsed. Still neither
http://test.bclary.com/tests/mozilla.org/js/js-test-driver-standards.html?test=js1_7/geniter/regress-349012-01.js;language=language;javascript1.7
nor
http://test.bclary.com/tests/mozilla.org/js/js-test-driver-standards.html?test=js1_7/geniter/regress-349012-01.js;language=type;text/javascript;version=1.7

work:

FAILED!: Error loading script

FAILED!: Error loading script
BUGNUMBER: 349012
STATUS: closing a generator fails to report error if GeneratorExit ignored

Comment 74

[Bob Clary [:bc] (inactive)]

the ;language=type;text/javascript;version=1.7 works for me in branch and trunk. I'm doing an update now to bring the site up to date with recent checkins. Try it with a nightly build and see if something in your debug build is broken.

Comment 75

[Igor Bukanov]

With 1.8.1 the tests show occasional errors, but that does not prevent to use venkman. I think the bug is in the debugger. For example, try to set a break point in the test case on the line actual += "Inner finally";. The debugger never reaches it. Instead it prints to stderr:

WARNING: NS_ENSURE_TRUE(prop_count) failed, file /home/igor/m/1.8.1/mozilla/js/jsd/jsd_xpc.cpp, line 2203
vnk: execution hook: function gen() in <http://test.bclary.com/tests/mozilla.org/js/js1_7/geniter/regress-349012-01.js> line 62
** ASSERTION FAILED: no cx in execution hook **
<top>
anonymous@58
jsdExecutionHook@299
[anonymous]@0
gc@187
test@73
[anonymous]@45
WARNING: NS_ENSURE_TRUE(prop_count) failed, file /home/igor/m/1.8.1/mozilla/js/jsd/jsd_xpc.cpp, line 2203
vnk: execution hook: function gen() in <http://test.bclary.com/tests/mozilla.org/js/js1_7/geniter/regress-349012-01.js> line 66
** ASSERTION FAILED: no cx in execution hook **
<top>
anonymous@58
jsdExecutionHook@299
[anonymous]@0
gc@187
test@73
[anonymous]@45

I suspect the debugger just not prepared to deal with close hooks scheduled by the GC. Another problem could be related to the fact that yield does not notify the debugger about the transfer of control adding to the confusion.

Comment 76

[Bob Clary [:bc] (inactive)]

1.8, 1.9 20060830 windows/mac*/linux still have
./js1_7/geniter/regress-349012-01.js:62: TypeError: yield from closing generator
and it appears the line number in the error message is 1 too low.

Comment 77

[Bob Clary [:bc] (inactive)]

I have locally switched  js1_7/geniter/regress-349012-01.js to  js1_7/geniter/regress-349012-01-n.js to deal with the error yield from closing generator. With this change all tests pass. Igor, if that is not the "right" thing to do, please let me know otherwise I will remove js1_7/geniter/regress-349012-01.js and check in js1_7/geniter/regress-349012-01-n.js.

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

Comment 78

[Igor Bukanov]

(In reply to comment #77)
> I have locally switched  js1_7/geniter/regress-349012-01.js to 
> js1_7/geniter/regress-349012-01-n.js to deal with the error yield from closing
> generator. With this change all tests pass. Igor, if that is not the "right"
> thing to do, please let me know

No this is not the right thing to do. Ideally we should extend the shell with a function that allows to capture error output from asynchronous GC-triggered finalize blocks or at least clear the exit status. In the mean time please add to the end of the test 
quit(0)  
when running in the shell.

Comment 79

[Bob Clary [:bc] (inactive)]

Thanks for the claryfication. ;-)

Checking in regress-349012-01.js;
/cvsroot/mozilla/js/tests/js1_7/geniter/regress-349012-01.js,v  <--  regress-349012-01.js
new revision: 1.3; previous revision: 1.2

Comment 80

[Bob Clary [:bc] (inactive)]

I need help interpreting the test results:

js1_7/geniter/regress-349012-01.js has been "failing" on the trunk since 20060901 which is "ok" by comment 78.

error reason: yield from closing generator actual Page: http://test.mozilla.com/tests/mozilla.org/js/js1_7/geniter/regress-349012-01.js Line: 67

js1_7/geniter/regress-349012-03.js and js1_7/geniter/regress-349012-04.js been "failing" on the trunk since 20060903, and failing on 1.8 since 20060906
 
generator recursively calling itself via send is an Error Expected value 'true', Actual value 'false'

Igor?

Comment 81

[Igor Bukanov]

Attachment: Tests suite changes to reflect bug 349362

The 3,4 tests currently fails since they assume a particular error message which bug 349362 changed. The patch changes the tests to check for TypeError instead not to depend on the exact error text.

Comment 82

[Bob Clary [:bc] (inactive)]

(In reply to comment #81)

thanks!
Checking in js1_7/geniter/regress-349012-03.js;
/cvsroot/mozilla/js/tests/js1_7/geniter/regress-349012-03.js,v  <--  regress-349012-03.js
new revision: 1.2; previous revision: 1.1
done
Checking in js1_7/geniter/regress-349012-04.js;
/cvsroot/mozilla/js/tests/js1_7/geniter/regress-349012-04.js,v  <--  regress-349012-04.js
new revision: 1.2; previous revision: 1.1
done

Comment 83

[Bob Clary [:bc] (inactive)]

Note known 1.8 browser only failure:

js1_7/geniter/regress-349012-01.js
 
error reason: yield from closing generator function gen() {try {try {yield 1;} finally {actual += "Inner finally";yield 2;}} finally {actual += ",Outer finally";}}