415922 - Exception from within JSNewEnumerateOp on JSENUMERATE_NEXT not supported

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Joachim Kuebart]

User-Agent:       Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.1; SV1; .NET CLR 2.0.50727; .NET CLR 3.0.04506.30; .NET CLR 1.1.4322)
Build Identifier: current CVS

If a JSClass's implementation of JSNewEnumerateOp throws an exception from the JSENUMERATE_NEXT operation, the exception is swallowed (culprit: jsiter.c:CallEnumeratorNext()).

This is not reproducible with a stock SpiderMonkey because the built-in js_EnumerateOp never throws on JSENUMERATE_NEXT.


Reproducible: Always

Steps to Reproduce:
1. Patch jsshell with the provided provoke.diff which adds a JSNewEnumerateOp to the object "it" that throws an exception upon JSENUMERATE_NEXT.
2. Run the "function f() { for (k in it) return k }".
3. Also try in a SpiderMonkey compiled with DEBUG_NOT_THROWING defined.

Actual Results:  
Because the exception is swallowed the function f() unexpectedly returns [object It] when run in a jsshell modified by provoke.diff. Worse, when "return k" is replaced by "print(k)", the iteration never terminates.

When SpiderMonkey is compiled with DEBUG_NOT_THROWING defined, the example stops with an JS_ASSERT in JSOP_SETRVAL at the "return" in the function f() above.


Expected Results:  
The exception thrown from within the iterator should be promoted to the running script.


The code in question seems to have been introduced in bug 354982.

Comment 1

[Joachim Kuebart]

Attachment: Add throwing JSNewEnumerateOp to jsshell's object "it" to provoke the error

This patch is _not_ for submission! It modifies the jsshell in a way only useful to provoke the error this bug documents.

However, this modification is supposed to represent legitimate use of SpiderMonkey by an embedding in the wild.

Comment 2

[Joachim Kuebart]

Attachment: Fix CallEnumeratorNext() to not swallow exceptions.

Modify CallEnumeratorNext() to return JS_FALSE instead of JS_TRUE after an exception inside a JSNewEnumerateOp. This patch fixes the described problem.

Comment 3

[Brendan Eich [:brendan]]

Comment on attachment 301665 [details] [diff] [review]
Fix CallEnumeratorNext() to not swallow exceptions.

Wow, easy fix -- thanks, Joachim. BTW, I gave you canconfirm and editbugs bugzilla privileges so your new bugs will be NEW, not UNCONFIRMED.

/be

Comment 4

[Brendan Eich [:brendan]]

Brian, can you get this in and also integrate the its_new_enumerate (with indentation nitpicking) for testing? Thanks,

/be

Comment 5

[Joachim Kuebart]

Attachment: Configurable behaviour of "its" enumerator in jsshell

This patch is an improved version of the one in comment 1. It adds a property "enum_fail" to the "it" object, allowing scripts to configure the behaviour of "its" iterator.

Comment 6

[Joachim Kuebart]

Attachment: A quick test script (not yet testsuite-ified)

Using the patch to jsshell from comment 5, this is an automatic test script.

Comment 7

[Joachim Kuebart]

(In reply to comment #3)
> (From update of attachment 301665 [details] [diff] [review])
> Wow, easy fix -- thanks, Joachim. BTW, I gave you canconfirm and editbugs
> bugzilla privileges so your new bugs will be NEW, not UNCONFIRMED.
> /be

Thanks for the privilege!

Joachim

Comment 8

[Brian Crowder]

Attachment: spacing nits and merged patch

Carrying over r/a+ since the jsiter.c change remains identical to the original patch.  js.c (NPOTB) changes are mostly whitespace, though I did rephrase the its_setProperty routine a bit.

Comment 9

[Brian Crowder]

landed the attachment from comment #8:
jsiter.c: 3.84
js.c:     3.191

Comment 10

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/public-failures.txt,v  <--  public-failures.txt
new revision: 1.40; previous revision: 1.39

/cvsroot/mozilla/js/tests/js1_7/iterable/regress-415922.js,v  <--  regress-415922.js
initial revision: 1.1

Comment 11

[Bob Clary [:bc] (inactive)]

v