407501 - JSOP_NEWINIT lacks SAVE_SP_AND_PC

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Igor Bukanov]

The changes to JSOP_NEWINIT case in the interpreter from bug 376957 missed SAVE_SP_AND_PC call before js_NewArrayObject/js_NewObject.

Comment 1

[Igor Bukanov]

Attachment: fix v1

Comment 2

[Igor Bukanov]

Attachment: fix v2

The new version uses  END_CASE(JSOP_NEW) to terminate JSOP_NEW as the code no longer shared with JSOP_NEWINIT and removes assignmnet to the object which is unnecessary since the bug 363530 was fixed.

Comment 3

[Igor Bukanov]

The regression introduced a GC hazard into the code.

Comment 4

[Igor Bukanov]

Attachment: fix v3

The new version adds LOAD_INTERRUPT_HANDLER to JSOP_NEWINIT as newObjectHook called during Object or array creation can reset the interrupt handler.

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 292236 [details] [diff] [review]
fix v3

Thanks, Igor.

Drivers: this is important to fix ASAP (GC safety).

/be

Comment 6

[Reed Loden [:reed]]

Landed this myself so we can respin nightlies now.

Checking in js/src/jsinterp.c;
/cvsroot/mozilla/js/src/jsinterp.c,v  <--  jsinterp.c
new revision: 3.390; previous revision: 3.389
done

Comment 7

[Bob Clary [:bc] (inactive)]

(In reply to comment #3)
> The regression introduced a GC hazard into the code.
> 

was this testable?

Comment 8

[Igor Bukanov]

(In reply to comment #7)
> was this testable?

Here is a test that without the patch leads to the segfault in jsshell:

gczeal(2);
var a = [[[[[[[0]]]]]]];
if (uneval(a).length == 0)
    throw "Unexpected result";

Comment 9

[Bob Clary [:bc] (inactive)]

Attachment: js1_5/extensions/regress-407501.js

I can't reproduce the bug with this. Igor, can you tweak it?

Comment 10

[Igor Bukanov]

The test shows the bug for me. Without the patch from comment 4:

~/m/trunk/mozilla/js/tests $ ./jsDriver.pl -e smdebug -l regress-407501.js  -t
-*- getting test list from user specified source.
-*- Testing engine 'smdebug'
-*- getting spidermonkey engine command.
-*- got './../src/Linux_All_DBG.OBJ/js '
-#- Executing 1 test(s).
-*- executing: ./../src/Linux_All_DBG.OBJ/js  -f ./shell.js -f ./regress-407501.js -f ./js-test-driver-end.js
*-* Testcase regress-407501.js failed:
Expected exit code 0, got 0
Testcase terminated with signal 11
Complete testcase output was:
Testcase produced no output!
-*- exit code 0, exit signal 11.
-*- Writing output to results-2007-12-13-133005-smdebug.html.
-#- Wrote failures to 'results-2007-12-13-133005-smdebug-failures.txt'.
-#- Wrote results to 'results-2007-12-13-133005-smdebug.html'.
-#- 1 test(s) failed

Using gczeal is essential in the test. Without it the bug can only be exposed if gc happens during Array allocation. This is hard to trigger, although possible as the following test demonstrates. The test tries to triggers the last-ditch GC: 

~/m/trunk/mozilla/js/src $ cat ~/m/y.js 

function test()
{
    var i, N = 10*1000;
    var a = Array(250);
    var bag = [];
    var src = 'for (i = 0; i != N; ++i) '+
              'bag.push('+a.join('[')+'0'+a.join(']')+');';
    eval(src);

    for (var i = 0; i != bag.length; ++i)
        check_array(bag[i]);
    
    function check_array(x)
    {
        for (var j = a.length - 1; j != 0; --j) {
            if (x.length != 1)
                throw "Bad array";
            x = x[0];
        }
        if (x !== 0)
            throw "Bad array";
    }
}    

test();
~/m/trunk/mozilla/js/src $ ./Linux_All_OPT.OBJ/js ~/m/y.js 
segmentation fault


Note: the test running time is few seconds.

Comment 11

[Igor Bukanov]

the last test reports out-of-memory with the trunk - this is expected.

Comment 12

[Bob Clary [:bc] (inactive)]

Checking in js1_5/extensions/regress-407501.js;
/cvsroot/mozilla/js/tests/js1_5/extensions/regress-407501.js,v  <--  regress-407501.js
initial revision: 1.1
done

Comment 13

[Bob Clary [:bc] (inactive)]

verified fixed 1.9.0

Comment 14

[Igor Bukanov]

Comment on attachment 292941 [details]
js1_5/extensions/regress-407501.js

Here is forgotten r+