355052 - Crash [@ NoSuchMethod] [@ js_ComputeThis] with valueOf: gc and __iterator__

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

js> ( {valueOf: gc} - [function(){}].__iterator__ )()

This usually crashes trying to dereference something near zero, but one I saw it jump to 0x20202020 and once I saw it try to dereference 0x80000007.

Comment 1

[Igor Bukanov]

Here is a simple example:

js>  ( {valueOf: gc} - [].a )()
 ( {valueOf: gc} - [].a )()
before 9232, after 9232, break 08129000
Segmentation fault (core dumped)

Comment 2

[Igor Bukanov]

This a bug in the emitter. Consider the following example:

var obj = {valueOf: gc };

function f() {
  ( obj * [].a )();
}

dis(f);
f();


Before crashing it prints the following bytecode for f:

main:
00000:  name "obj"
00003:  name "Object"
00006:  pushobj
00007:  newinit
00008:  one
00009:  initprop "a"
00012:  endinit
00013:  getprop "a"
00016:  mul
00017:  group
00018:  pushobj
00019:  call 0
00022:  pop
00023:  stop

Source notes:
  0:     0 [   0] newline 
  1:    13 [  13] xdelta  
  2:    13 [   0] pcbase   offset 10
  4:    19 [   6] pcbase   offset 19

here pushobj at line 18 pushes to the stack the last value of obj register. In this case it is temporary left from getprop 013 which is already GC-ed. Something is badly broken regarding that pushobj. 

Comment 3

[Igor Bukanov]

This a bug in js_EmitTree when it deals with the TOK_LP case. It misses the fact that at line 5792 pn2 can be arbitrary expression.

 

Comment 4

[Brendan Eich [:brendan]]

Regression in patch for bug 320032, which reopened bug 41864.  JSOP_GROUP must null the |obj| register if the expression really needed to be parenthesized.  This points to the fix: the parser should eliminate the TOK_RP parenthesizing parent of any member expression.

/be

Comment 5

[Brendan Eich [:brendan]]

Attachment: fix

Comment 6

[Igor Bukanov]

Comment on attachment 240870 [details] [diff] [review]
fix

Now it works as expected.

Comment 7

[Igor Bukanov]

BTW, Rhino is significantly different from SM regarding implementing call bytecode. There the emitter generates special bytecodes to compute both this and function for all syntactical cases of () operator before calling this. It seems cleaner then various "obj = NULL".

Comment 8

[Brendan Eich [:brendan]]

Yes, SpiderMonkey influenced Rhino early on, and didn't keep up with its child ;-) -- file a followup bug?

/be

Comment 9

[Brendan Eich [:brendan]]

Safe fix, candidate for now, or if not, for the patch release.  Fixed on trunk:

Checking in jsinterp.c;
/cvsroot/mozilla/js/src/jsinterp.c,v  <--  jsinterp.c
new revision: 3.295; previous revision: 3.294
done
Checking in jsparse.c;
/cvsroot/mozilla/js/src/jsparse.c,v  <--  jsparse.c
new revision: 3.252; previous revision: 3.251
done

/be

Comment 10

[Mike Schroepfer]

Comment on attachment 240870 [details] [diff] [review]
fix

Approved for RC2.

Comment 11

[Brendan Eich [:brendan]]

Checking in jsinterp.c;
/cvsroot/mozilla/js/src/jsinterp.c,v  <--  jsinterp.c
new revision: 3.181.2.67; previous revision: 3.181.2.66
done
Checking in jsparse.c;
/cvsroot/mozilla/js/src/jsparse.c,v  <--  jsparse.c
new revision: 3.142.2.64; previous revision: 3.142.2.63
done

/be

Comment 12

[Brendan Eich [:brendan]]

The patch for this bug caused bug 355101.

/be

Comment 13

[Bob Clary [:bc] (inactive)]

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

Comment 14

[Bob Clary [:bc] (inactive)]

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

Comment 15

[Bob Clary [:bc] (inactive)]

Attachment: js1_7/iterable/regress-355052-03.js

Comment 16

[Bob Clary [:bc] (inactive)]

verified fixed 1.8 1.9 20061002 windows/linux 1.8 macppc

Comment 17

[Daniel Veditz [:dveditz]]

Already in 1.8.1, clearning nomination flags.

Comment 18

[Bob Clary [:bc] (inactive)]

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

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

/cvsroot/mozilla/js/tests/js1_7/extensions/regress-355052-03.js,v  <--  regress-355052-03.js

Comment 19

[Bob Clary [:bc] (inactive)]

On the trunk these exceptions have changed from 'TypeError: NaN is not a function' 
to 'TypeError: - is not a function' for 01, 02 and 'TypeError: * is not a function' for 03. Range is 2006-12-27 to 2006-12-28 and looks like bug 363536.

Do we care or should I rewrite the test to handle either case?

Comment 20

[Brendan Eich [:brendan]]

These look like regressions to me. Igor?

/be