closing a generator fails to report error if GeneratorExit ignored

VERIFIED FIXED in mozilla1.8.1

Status

()

Core
JavaScript Engine
P1
normal
VERIFIED FIXED
12 years ago
11 years ago

People

(Reporter: Igor Bukanov, Assigned: brendan)

Tracking

({verified1.8.1})

Trunk
mozilla1.8.1
verified1.8.1
Points:
---
Bug Flags:
blocking1.8.1 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Fx2b2 rider][181approval pending])

Attachments

(9 attachments, 8 obsolete attachments)

fix
4.28 KB, patch
Igor Bukanov
: review+
mrbkap
: superreview+
Details | Diff | Splinter Review
13.10 KB, patch
brendan
: review+
brendan
: superreview+
Details | Diff | Splinter Review
12.69 KB, patch
Details | Diff | Splinter Review
7.97 KB, patch
mrbkap
: review+
Igor Bukanov
: superreview+
Details | Diff | Splinter Review
7.88 KB, patch
Details | Diff | Splinter Review
16.68 KB, patch
brendan
: review+
brendan
: superreview+
Mike Schroepfer
: approval1.8.1+
Details | Diff | Splinter Review
10.60 KB, text/plain
Details
1.90 KB, patch
Details | Diff | Splinter Review
3.05 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

12 years ago
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.
(Reporter)

Comment 1

12 years ago
(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.
(Reporter)

Comment 2

12 years ago
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
(Reporter)

Comment 3

12 years ago
(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?   
(Assignee)

Comment 4

12 years ago
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
(Reporter)

Comment 5

12 years ago
(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.
(Assignee)

Comment 6

12 years ago
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
Assignee: general → brendan
Flags: blocking1.8.1?
Priority: -- → P1
Target Milestone: --- → mozilla1.8.1
(Assignee)

Comment 7

12 years ago
Created attachment 234258 [details] [diff] [review]
fix

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

/be
Attachment #234258 - Flags: superreview?(mrbkap)
Attachment #234258 - Flags: review?(igor.bukanov)

Comment 8

12 years ago
Putting on the 1.8.1/FF2 radar...
Flags: blocking1.8.1? → blocking1.8.1+
(Assignee)

Updated

12 years ago
Summary: JS should not allow yield inside finally → closing a generator fails to report error if GeneratorExit ignored

Updated

12 years ago
Attachment #234258 - Flags: superreview?(mrbkap) → superreview+
(Reporter)

Comment 9

12 years ago
(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.
(Reporter)

Comment 10

12 years ago
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.
Priority: P1 → --
Target Milestone: mozilla1.8.1 → ---
(Assignee)

Comment 11

12 years ago
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
Status: NEW → ASSIGNED
(Reporter)

Comment 12

12 years ago
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.
(Reporter)

Comment 13

12 years ago
(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?
(Assignee)

Comment 14

12 years ago
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
(Reporter)

Comment 15

12 years ago
Comment on attachment 234258 [details] [diff] [review]
fix

+ for cleanup, not ignoring finally!
Attachment #234258 - Flags: review?(igor.bukanov) → review+
(Reporter)

Comment 16

12 years ago
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. 
(Assignee)

Comment 17

12 years ago
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

12 years ago
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.
(Assignee)

Comment 19

12 years ago
Created attachment 234340 [details] [diff] [review]
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
Attachment #234340 - Flags: superreview?(mrbkap)
Attachment #234340 - Flags: review?(igor.bukanov)
(Assignee)

Comment 20

12 years ago
Created attachment 234341 [details] [diff] [review]
diff -w version of last attachment
(Reporter)

Comment 21

12 years ago
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.
Attachment #234340 - Flags: review?(igor.bukanov) → review+
(Reporter)

Comment 22

12 years ago
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.
(Reporter)

Comment 23

12 years ago
(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>  
(Assignee)

Comment 24

12 years ago
(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

Updated

12 years ago
Attachment #234340 - Flags: superreview?(mrbkap) → superreview+
(Assignee)

Comment 25

12 years ago
Created attachment 234452 [details] [diff] [review]
Last patch, plus throw on nesting generator execution
Attachment #234340 - Attachment is obsolete: true
Attachment #234452 - Flags: superreview?(mrbkap)
Attachment #234452 - Flags: review?(igor.bukanov)
(Assignee)

Comment 26

12 years ago
Created attachment 234453 [details] [diff] [review]
diff -w version of last attachment

For easier reviewing.

/be
Attachment #234341 - Attachment is obsolete: true

Updated

12 years ago
Attachment #234452 - Flags: superreview?(mrbkap) → superreview+
(Assignee)

Comment 27

12 years ago
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
(Reporter)

Comment 28

12 years ago
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.
(Assignee)

Comment 29

12 years ago
Created attachment 234496 [details] [diff] [review]
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
Attachment #234452 - Attachment is obsolete: true
Attachment #234496 - Flags: superreview+
Attachment #234496 - Flags: review+
Attachment #234452 - Flags: review?(igor.bukanov)
(Assignee)

Comment 30

12 years ago
Created attachment 234497 [details] [diff] [review]
diff -w version of last attachment

For reference.

/be
Attachment #234453 - Attachment is obsolete: true
(Assignee)

Comment 31

12 years ago
Fixed on trunk.  Preparing all-in-one branch patch.

/be
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
(Assignee)

Comment 32

12 years ago
Created attachment 234526 [details] [diff] [review]
all-in-one 1.8 branch patch created by cvs up -j
Attachment #234526 - Flags: superreview+
Attachment #234526 - Flags: review+
Attachment #234526 - Flags: approval1.8.1?
(Reporter)

Comment 33

12 years ago
(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.
(Assignee)

Comment 34

12 years ago
(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
(Assignee)

Comment 35

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

/be
(Reporter)

Comment 36

12 years ago
(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. 




(Assignee)

Comment 37

12 years ago
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Updated

12 years ago
Attachment #234526 - Attachment is obsolete: true
Attachment #234526 - Flags: superreview+
Attachment #234526 - Flags: review+
Attachment #234526 - Flags: approval1.8.1?
(Assignee)

Updated

12 years ago
Status: REOPENED → ASSIGNED
Priority: -- → P1
Target Milestone: --- → mozilla1.8.1
(Assignee)

Comment 38

12 years ago
Created attachment 234529 [details] [diff] [review]
fix generator state coding and testing

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

/be
Attachment #234529 - Flags: superreview?(mrbkap)
Attachment #234529 - Flags: review?(igor.bukanov)
(Assignee)

Comment 39

12 years ago
Created attachment 234530 [details] [diff] [review]
diff -w version of last attachment
Comment on attachment 234529 [details] [diff] [review]
fix generator state coding and testing

Sorry, I should have seen this before.
Attachment #234529 - Flags: superreview?(mrbkap) → superreview+
(Assignee)

Comment 41

12 years ago
(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
(Reporter)

Comment 42

12 years ago
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.
(Assignee)

Comment 43

12 years ago
Created attachment 234533 [details] [diff] [review]
fix per Igor's last comment
Attachment #234529 - Attachment is obsolete: true
Attachment #234529 - Flags: review?(igor.bukanov)
(Assignee)

Comment 44

12 years ago
Created attachment 234534 [details] [diff] [review]
diff -w version of last attachment
Attachment #234530 - Attachment is obsolete: true
(Assignee)

Comment 45

12 years ago
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
Attachment #234533 - Flags: superreview?(igor.bukanov)
Attachment #234533 - Flags: review?(mrbkap)
(Reporter)

Comment 46

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

:)
Attachment #234533 - Flags: superreview?(igor.bukanov) → superreview+
(Reporter)

Comment 47

12 years ago
(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. 

Updated

12 years ago
Attachment #234533 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 48

12 years ago
Fixed, I hope.  Branch patch in a bit.

/be
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago12 years ago
Resolution: --- → FIXED
(Assignee)

Comment 49

12 years ago
Created attachment 234555 [details] [diff] [review]
combined 1.8 branch patch
Attachment #234555 - Flags: superreview+
Attachment #234555 - Flags: review+
Attachment #234555 - Flags: approval1.8.1?
(Assignee)

Comment 50

12 years ago
Created attachment 234556 [details] [diff] [review]
combined 1.8 branch patch

Argh, didn't cp before attaching.

/be
Attachment #234555 - Attachment is obsolete: true
Attachment #234555 - Flags: approval1.8.1?
(Assignee)

Updated

12 years ago
Attachment #234555 - Flags: superreview+
Attachment #234555 - Flags: review+
(Assignee)

Updated

12 years ago
Attachment #234556 - Flags: superreview+
Attachment #234556 - Flags: review+
Attachment #234556 - Flags: approval1.8.1?
(Assignee)

Updated

12 years ago
Blocks: 326466

Comment 51

12 years ago
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
Flags: in-testsuite+
Whiteboard: [181approval pending]
Whiteboard: [181approval pending] → [Fx2b2 rider][181approval pending]

Comment 52

12 years ago
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.
Attachment #234556 - Flags: approval1.8.1? → approval1.8.1+

Comment 53

12 years ago
Igor, is there anyway to catch the GeneratorExit ignored by generator function error, or did I misname this test?
(Assignee)

Comment 54

12 years ago
(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
(Reporter)

Comment 55

12 years ago
> 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.
(Assignee)

Comment 56

12 years ago
(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
(Reporter)

Comment 57

12 years ago
(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. 
(Reporter)

Comment 58

12 years ago
(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.  
(Assignee)

Comment 59

12 years ago
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
(Assignee)

Comment 60

12 years ago
(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
(Assignee)

Comment 61

12 years ago
Fixed on the 1.8 branch.

/be
Keywords: fixed1.8.1

Comment 62

12 years ago
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
(Reporter)

Comment 63

12 years ago
(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

12 years ago
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

12 years ago
Created attachment 235707 [details]
funkytown stacks
(Reporter)

Comment 66

12 years ago
Created attachment 235711 [details] [diff] [review]
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

12 years ago
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.
(Reporter)

Comment 68

12 years ago
(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

12 years ago
(In reply to comment #68)

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

Comment 70

12 years ago
(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

(Reporter)

Comment 72

12 years ago
(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
(Reporter)

Comment 73

12 years ago
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

12 years ago
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.

(Reporter)

Comment 75

12 years ago
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

12 years ago
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

12 years ago
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
Status: RESOLVED → VERIFIED
Keywords: fixed1.8.1 → verified1.8.1
(Reporter)

Comment 78

12 years ago
(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

12 years ago
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

12 years ago
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?
(Reporter)

Comment 81

12 years ago
Created attachment 237604 [details] [diff] [review]
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

12 years ago
(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

11 years ago
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";}} 
You need to log in before you can comment on or make changes to this bug.