355025 - Regression: Iterator() does not work any longer

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

My patch for security-restricted bug 354750 introduced a regression. Now Iterator(obj) would not construct a default iterator but throw an exception about value-is-not-a-function, only new Iterator(obj) would.

Comment 1

[Igor Bukanov]

The test case is trivial:

js> Iterator([])
Iterator([])
aaaa
typein:1: TypeError: Iterator([]) is not a function
js> 

Comment 2

[Igor Bukanov]

Attachment: Fix v1

A better solution would be to call js_ValueToIterator when Iterator is called as a function, but that would require to pass JSITER_ENUMERATE from the interpreter to js_ValueToIterator and increase the patch. So the cleanup should wait bug 354982.

Comment 3

[Brendan Eich [:brendan]]

Comment on attachment 240814 [details] [diff] [review]
Fix v1

>     id = ATOM_TO_JSID(cx->runtime->atomState.iteratorAtom);
>-    return JS_GetMethodById(cx, obj, id, &obj, &fval) &&
>-           js_InternalCall(cx, obj, fval, argc - 1, argv + 1, rval);
>+    if (!JS_GetMethodById(cx, obj, id, &obj2, &fval))

I made the same mistake in the past, and should have caught this.  FWIW, I'll review one more time, but my record isn't good.

>+        return JS_FALSE;
>+    
>+    if (JSVAL_IS_VOID(fval)) {
>+        /* Fail over to the default enumerating native iterator. */
>+        keyonly = JS_FALSE;
>+        return js_ValueToBoolean(cx, argv[1], &keyonly) &&
>+               js_NewNativeIterator(cx, obj,
>+                                    keyonly ? 0 : JSITER_FOREACH,
>+                                    rval);
>+    }

The lack of Object.prototype.__iterator__ requires this, yeah.  Is it needed anywhere else?

Drivers: this is needed as a < 1-day turnaround regression fix for 1.8.1.

/be

Comment 4

[Igor Bukanov]

(In reply to comment #3)
> The lack of Object.prototype.__iterator__ requires this, yeah.  Is it needed
> anywhere else?

Well, after creating the initial patch for bug 354982 I think this is the only place as there are no other places besides js_NewNativeIterator that create iterator instances. 

Comment 5

[Igor Bukanov]

I committed the patch from comment 2 to the trunk:

Checking in jsiter.c;
/cvsroot/mozilla/js/src/jsiter.c,v  <--  jsiter.c
new revision: 3.46; previous revision: 3.45
done

Comment 6

[Mike Schroepfer]

Comment on attachment 240814 [details] [diff] [review]
Fix v1

Approved for RC2.

Comment 7

[Igor Bukanov]

I committed the patch from comment 2 to MOZILLA_1_8_BRANCH:

Checking in jsiter.c;
/cvsroot/mozilla/js/src/jsiter.c,v  <--  jsiter.c
new revision: 3.17.2.21; previous revision: 3.17.2.20
done

Comment 8

[Bob Clary [:bc] (inactive)]

Attachment: js1_7/iterable/regress-355025.js

Comment 9

[Bob Clary [:bc] (inactive)]

verified fixed 1.8 1.9 20061002 windows/linux 1.8 macppc

Comment 10

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/js1_7/iterable/regress-355025.js,v  <--  regress-355025.js