354945 - Crash [@ js_Enumerate] with "new Iterator"

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

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

Comment 1

[Igor Bukanov]

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. 

Comment 2

[Brendan Eich [:brendan]]

(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

Comment 3

[Igor Bukanov]

(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.

Comment 4

[Brendan Eich [:brendan]]

(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

Comment 5

[Brendan Eich [:brendan]]

(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

Comment 6

[Igor Bukanov]

Attachment: Fix v1

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.

Comment 7

[Igor Bukanov]

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;

Comment 8

[Igor Bukanov]

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 9

[Brendan Eich [:brendan]]

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

Comment 10

[Igor Bukanov]

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 11

[Mike Schroepfer]

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

Let's wrap up the JS changes in one batch.  Approved for RC2.

Comment 12

[Brendan Eich [:brendan]]

$ 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

Comment 13

[Daniel Veditz [:dveditz]]

Not needed in 1.8.0.8, right?

Comment 14

[Igor Bukanov]

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

Yes.

Comment 15

[Bob Clary [:bc] (inactive)]

Attachment: js1_7/iterable/regress-354945-01.js

Comment 16

[Bob Clary [:bc] (inactive)]

Attachment: js1_7/iterable/regress-354945-02.js

Comment 17

[Bob Clary [:bc] (inactive)]

verified fixed 1.8 1.9 20061002 windows/linux 1.8 macppc

Comment 18

[Bob Clary [:bc] (inactive)]

/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