obj = {}; obj.__iterator__ = function(){ }; for(t in (new Iterator(obj))) { } Segmentation fault Contrast with iterating over the object without using "new Iterator": js> obj = {}; obj.__iterator__ = function(){ }; for(t in (obj)) { } typein:1: TypeError: obj.__iterator__ returned a primitive value In both opt in debug, the crash is a segmentation fault due to attempting to dereference memory at 0x80000004. Here's a debug stack trace: Exception: EXC_BAD_ACCESS (0x0001) Codes: KERN_INVALID_ADDRESS (0x0001) at 0x80000004 Thread 0 Crashed: 0 js 0x0004cd30 js_Enumerate + 1288 (jsobj.c:4000) 1 js 0x00105574 iterator_next + 644 (jsiter.c:237) 2 js 0x001068ec js_CallIteratorNext + 1224 (jsiter.c:519) 3 js 0x00098948 js_Interpret + 12304 (jsinterp.c:2759) 4 js 0x00094400 js_Execute + 936 (jsinterp.c:1618) 5 js 0x00021578 JS_ExecuteScript + 64 (jsapi.c:4256) 6 js 0x00003084 Process + 904 (js.c:265) 7 js 0x00003c4c ProcessArgs + 2304 (js.c:487) 8 js 0x0000a03c main + 640 (js.c:3088) 9 js 0x000023e8 _start + 340 (crt.c:272) 10 js 0x00002290 start + 60

The problem is that Iterator constructor in jsiter.c assumes that it always would be used as function and never calls js_NewNativeIterator assuming that __iterator__ property would provide the necessary result. Now since in the example the iterator is called as constructor and the __iterator__ is function returning a primitive value (undefined), so the end result is constructed but not initialized iterator. The right fix AFAICS is to check for constructor call and if so make a default iterator ignoring the value of __iterator__ property. That is, new Iterator(obj) should construct the default iterator, while Iterator(obj) should build the iterator from object using __iterator__ property. Note also that such fix may interfere with the patch for bug 354499 so I would appreciate commenets there.

(In reply to comment #1) > The right fix AFAICS is to check for constructor call and if so make a default > iterator ignoring the value of __iterator__ property. That is, new > Iterator(obj) should construct the default iterator, while Iterator(obj) should > build the iterator from object using __iterator__ property. That's right. > Note also that such fix may interfere with the patch for bug 354499 so I would > appreciate commenets there. How about a spot-fix here first? Still timef or 1.8.1. /be

(In reply to comment #2) > > How about a spot-fix here first? Still time for 1.8.1. Then there is a time to fix the leakage of enumerating iterators to script thus addressing partially bug 354499. An easy fix for that AFAICS would involve removing Object.prototype.__iterator__ to state that for (i in obj) uses the default enumerator searching the prototype if obj does not have __iterator__. This would simplify code with a fix that involves removal of code. More complex simplifications can wait for FF 2.0.1.

(In reply to comment #3) > (In reply to comment #2) > > > > How about a spot-fix here first? Still time for 1.8.1. > > Then there is a time to fix the leakage of enumerating iterators to script thus > addressing partially bug 354499. Sorry, I missed your comment there in recent bugmail, or left it for after I'd finished other work, at any rate. Bad move on my part, but if we can get a patch in today, we may be in time for 1.8.1. > An easy fix for that AFAICS would involve > removing Object.prototype.__iterator__ to state that for (i in obj) uses the > default enumerator searching the prototype if obj does not have __iterator__. > This would simplify code with a fix that involves removal of code. Is this sufficient to fix this bug? The test in comment 0 doesn't hack on any prototype __iterator__. Is it necessary? For ES4/JS2 (with change of name to use namespaces instead of __ugly__ names), we want Object.prototype to contain the default iterator-getter. /be

(In reply to comment #4) > For ES4/JS2 (with change of name to > use namespaces instead of __ugly__ names), we want Object.prototype to contain > the default iterator-getter. For the record, and see bug 354750, we don't want this. /be

The patch splits js_NewNativeIterator into js_NewNativeIterator and InitNativeIterator and calls the latter from the constructor. The patch also removes js_DefaultIterator to avoid useless conversions from flags to bool and back to flags.

With the patch the following test cases also passes demonstrating usability of calling new iterator directly: Array.prototype.pop2 = function() { return [this.pop(), this.pop()]; } var emptyArrayProperties = []; for (var i in []) { emptyArrayProperties.push(i); } if (emptyArrayProperties.length != 1) throw "Unexpected emptyArrayProperties.length: "+emptyArrayProperties.length; if (emptyArrayProperties != "pop2") throw "Unexpected emptyArrayProperties[0]: "+emptyArrayProperties[0]; Object.prototype.__iterator__ = function(keyonly) { return new Iterator(this, keyonly); } var emptyArrayProperties = []; for (var i in []) { emptyArrayProperties.push(i); } if (emptyArrayProperties.length != 0) throw "Unexpected emptyArrayProperties.length: "+emptyArrayProperties.length;

The interesting thing is that Object.prototype.__iterator__ = function(keyonly) { return new Iterator(this, keyonly); } speeds up enumeration over array instances by factor of 2.5 since it avoids number->string conversion. For example, the following test case with Object.prototype.__iterator__ commented out runs 2.5 slower then with the code present: Object.prototype.__iterator__ = function(keyonly) { return new Iterator(this, keyonly); } function test(N) { var i = N; var array = Array(N); for (i = 0; i != N; ++i) array[i] = i; var now = Date.now; var time = now(); var sum = 0; for (i in array) { sum += array[i]; } time = now() - time; if (sum != N * (N - 1) / 2) throw "sum deviation:"+(sum - N * (N - 1) / 2); return time; } var time = test(100 * 1000); print(time);

Comment on attachment 240766 [details] [diff] [review] Fix v1 Nice! Yes, iteration rules because it avoids key to string conversion. This is a s-s bugfix and a change we will want in any source release of js1.7. I'm nominating it for 1.8.1. The patch is not as big as it looks, because of the common subroutining going on. If it can be landed on the trunk ASAP and meets approval of the fuzzer and testsuite, it could squeak into 1.8.1. Otherwise 1.8.1.1. /be

I committed the patch from comment 6 to the trunk: Checking in jsiter.c; /cvsroot/mozilla/js/src/jsiter.c,v <-- jsiter.c new revision: 3.45; previous revision: 3.44 done Checking in jsiter.h; /cvsroot/mozilla/js/src/jsiter.h,v <-- jsiter.h new revision: 3.15; previous revision: 3.14 done

Comment on attachment 240766 [details] [diff] [review] Fix v1 Let's wrap up the JS changes in one batch. Approved for RC2.

$ cvs ci -m"Checking in for Igor, bug 354945, a=schrep." jsiter.[ch] Checking in jsiter.c; /cvsroot/mozilla/js/src/jsiter.c,v <-- jsiter.c new revision: 3.17.2.20; previous revision: 3.17.2.19 done Checking in jsiter.h; /cvsroot/mozilla/js/src/jsiter.h,v <-- jsiter.h new revision: 3.6.2.9; previous revision: 3.6.2.8 done /be

Not needed in 1.8.0.8, right?

(In reply to comment #13) > Not needed in 1.8.0.8, right? > Yes.

verified fixed 1.8 1.9 20061002 windows/linux 1.8 macppc

/cvsroot/mozilla/js/tests/js1_7/extensions/regress-354945-01.js,v <-- regress-354945-01.js /cvsroot/mozilla/js/tests/js1_7/extensions/regress-354945-02.js,v <-- regress-354945-02.js