350809 - yield in xml-filtering-predicate causes assertion failure

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Jesse Ruderman]

javascript:(function(){ <x/>.(yield 4) })().next();

Assertion failure: fp->sp == sp, at /Users/admin/trunk/mozilla/js/src/jsinterp.c:5718

Comment 1

[Brendan Eich [:brendan]]

Attachment: fix to prevent yield from filtering predicate

If bug 309894 were fixed we could remove this error, but that bug won't be fixed in js1.7.

If this is considered too much for a hard case, I could make a runtime error when yield actually unwinds js_Interpret from called from JSOP_FILTER.  Thoughts?

/be

Comment 2

[Brendan Eich [:brendan]]

Attachment: altern-a-patch

This catches the error only at runtime, which is less helpful and may be missed if code coverage is incomplete -- but the patch is smaller.

/be

Comment 3

[Igor Bukanov]

Comment on attachment 237082 [details] [diff] [review]
altern-a-patch

>+MSG_DEF(JSMSG_YIELD_FROM_FILTER,      217, 0, JSEXN_TYPEERR, "yield disallowed from filtering predicate")

What about using more honest "yield from filtering predicate is not supported" with JSEXN_INTERNALERR?

Comment 4

[Igor Bukanov]

(In reply to comment #1)
> If this is considered too much for a hard case, I could make a runtime error
> when yield actually unwinds js_Interpret from called from JSOP_FILTER. 
> Thoughts?

A runtime error emphases that this is a temporary interpreter implementation, not a permanent syntax error IMO. 

Comment 5

[Brendan Eich [:brendan]]

Attachment: altern-a-patch to commit

I'm checking this in.

/be

Comment 6

[Brendan Eich [:brendan]]

Fixed on trunk.

/be

Comment 7

[Brendan Eich [:brendan]]

Not sure why this is baking, esp. compared to other fixes that aren't.  This is adding a new error to a case that assertbotches debug builds and probably crashes release ones, and it can't be making things worse since the condition tested to throw the error instead of crashing is exactly the condition that leads to the crash.

/be

Comment 8

[Brendan Eich [:brendan]]

This bug is quite comparable in scope and risk of its fix to bug 350279, whose patch was approved without extra baking.

/be

Comment 9

[Mike Schroepfer]

Comment on attachment 237132 [details] [diff] [review]
altern-a-patch to commit

a=schrep for drivers.

Comment 10

[Brendan Eich [:brendan]]

Fixed on the 1.8 branch.

/be

Comment 11

[Bob Clary [:bc] (inactive)]

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

Comment 12

[Bob Clary [:bc] (inactive)]

verified fixed 1.8 1.9 20060909 windows/mac*/linux