422348 - Proper overflow error reporting

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Igor Bukanov]

+++ This bug was initially created as a clone of Bug #422137 +++

Currently SpiderMonkey, when in various places it checks for a possible overflow when allocating temporary buffers, it calls js_ReportOutOfMemory when it detects the condition. But this is wrong since overflow does not indicate that the engine is out-of-memory. Rather it indicates an input that the engine can not handle and should be reported as such. 

Example:

js> try { Array(1 << 30).sort(); } catch (e) { print("Captured: "+uneval(e)); } 
typein:4: out of memory
 
when it is expected that script prints "Captured: ..."

Comment 1

[Igor Bukanov]

Attachment: v1

The patch introduces js_ReportAllocSizeOverflow and uses it to report over-sized allocation attempts.

Withe the patch the following test cases fails since they expect out-of-memory behavior:

js1_5/Function/regress-338001.js
js1_5/Array/regress-157652.js
js1_5/Regress/regress-303213.js

Here the test run output:

~/m/ff/mozilla/js/tests $ ./jsDriver.pl -e smdebug -L slow-n.tests performance.tests base-failures.txt -l ~/s/x.txt -t
-*- getting test list from user specified source.
-*- getting negative list from user specified source.
-*- 0 of 3 tests will be skipped.
-*- 243 skip tests were not actually part of the test list.
-*- Testing engine 'smdebug'
-*- getting spidermonkey engine command.
-*- got './../src/Linux_All_DBG.OBJ/js '
-#- Executing 3 test(s).
-*- executing: ./../src/Linux_All_DBG.OBJ/js  -f ./shell.js -f ./js1_5/shell.js -f ./js1_5/Function/shell.js -f ./js1_5/Function/regress-338001.js -f ./js-test-driver-end.js
-*- Test case expects exit code 0
-*- Test case expects exit code 5
*-* Testcase js1_5/Function/regress-338001.js failed:
Bug Number 338001
Expected exit code 5, got 3
Testcase terminated with signal 0
Complete testcase output was:
./js1_5/Function/regress-338001.js:61: InternalError: allocation size overflow
BUGNUMBER: 338001
STATUS: integer overflow in jsfun.c:Function
--- NOTE: IN THIS TESTCASE, WE EXPECT EXIT CODE 0 ---
--- NOTE: IN THIS TESTCASE, WE EXPECT EXIT CODE 5 ---

-*- exit code 3, exit signal 0.
-*- executing: ./../src/Linux_All_DBG.OBJ/js  -f ./shell.js -f ./js1_5/shell.js -f ./js1_5/Array/shell.js -f ./js1_5/Array/regress-157652.js -f ./js-test-driver-end.js
-*- Test case expects exit code 5
*-* Testcase js1_5/Array/regress-157652.js failed:
Bug Number 157652
Expected exit code 5, got 3
Testcase terminated with signal 0
Complete testcase output was:
./js1_5/Array/regress-157652.js:133: InternalError: allocation size overflow
BUGNUMBER: 157652
STATUS: Testing that Array.sort() doesn't crash on very large arrays
--- NOTE: IN THIS TESTCASE, WE EXPECT EXIT CODE 5 ---

-*- exit code 3, exit signal 0.
-*- executing: ./../src/Linux_All_DBG.OBJ/js  -f ./shell.js -f ./js1_5/shell.js -f ./js1_5/Regress/shell.js -f ./js1_5/Regress/regress-303213.js -f ./js-test-driver-end.js
-*- Test case expects exit code 5
*-* Testcase js1_5/Regress/regress-303213.js failed:
Bug Number 303213
Expected exit code 5, got 3
Testcase terminated with signal 0
Complete testcase output was:
./js1_5/Regress/regress-303213.js:74: InternalError: allocation size overflow
BUGNUMBER: 303213
STATUS: integer overflow in js
STATUS: This bug passes if no crash occurs
--- NOTE: IN THIS TESTCASE, WE EXPECT EXIT CODE 5 ---
STATUS: done generating

-*- exit code 3, exit signal 0.
-*- Writing output to results-2008-03-12-163654-smdebug.html.
-#- Wrote failures to 'results-2008-03-12-163654-smdebug-failures.txt'.
-#- Wrote results to 'results-2008-03-12-163654-smdebug.html'.
-#- 3 test(s) failed

Comment 2

[Bob Clary [:bc] (inactive)]

The tests can be modified to catch and test the exception and to add 0 as an expected exit code. This should work on the branch as well as trunk. Igor, sound ok?

Comment 3

[Mike Shaver (:shaver emeritus)]

You can't catch OOM on the branch; will that still work?

Comment 4

[Bob Clary [:bc] (inactive)]

branch would fail as it always has. The difference would be that the branch would not show a failure if the test didn't run out of memory but these tests are really expecting no crash as a pass condition and not an out of memory condition. I think that is ok.

Comment 5

[Igor Bukanov]

(In reply to comment #2)
> The tests can be modified to catch and test the exception and to add 0 as an
> expected exit code. This should work on the branch as well as trunk. Igor,
> sound ok?

This indeed the right solution given that the tests are crash tests.

Comment 6

[Mike Shaver (:shaver emeritus)]

Comment on attachment 308875 [details] [diff] [review]
v1

>+extern void
>+js_ReportAllocSizeOverflow(JSContext *cx);

We probably want this to be JSAPI as well, so that host objects can report properly in these cases as well.

JS_ReportAllocationOverflow?  One more char in length, but I think reads better.

r=shaver if you agree with those, but we can discuss it more if you want!

Comment 7

[Igor Bukanov]

Attachment: v2

The new version renames the reporting function to js_ReportAllocationOverflow (which is indeed a better name) and exposes the functionality as a public API .

Comment 8

[Mike Shaver (:shaver emeritus)]

Comment on attachment 308903 [details] [diff] [review]
v2

a=shaver for making class of errors handleable by Firebug and otherwise.

Comment 9

[Igor Bukanov]

I checked in the patch from the comment 7 to the trunk:

http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&branch=HEAD&cvsroot=%2Fcvsroot&date=explicit&mindate=1205363029&maxdate=1205363242&who=igor%25mir2.org

Comment 10

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/public-failures.txt,v  <--  public-failures.txt
new revision: 1.55; previous revision: 1.54

/cvsroot/mozilla/js/tests/js1_5/Function/regress-338001.js,v  <--  regress-338001.js
new revision: 1.3; previous revision: 1.2

/cvsroot/mozilla/js/tests/js1_5/Array/regress-157652.js,v  <--  regress-157652.js
new revision: 1.18; previous revision: 1.17

/cvsroot/mozilla/js/tests/js1_5/Regress/regress-303213.js,v  <--  regress-303213.js
new revision: 1.5; previous revision: 1.4

Comment 11

[Bob Clary [:bc] (inactive)]

an additional test exhibited the size allocation overflow.

/cvsroot/mozilla/js/tests/js1_5/Array/regress-330812.js,v  <--  regress-330812.js
new revision: 1.4; previous revision: 1.3

Comment 12

[Bob Clary [:bc] (inactive)]

verified linux|mac|windows

/cvsroot/mozilla/js/tests/js1_5/Regress/regress-422348.js,v  <--  regress-422348.js
initial revision: 1.1

Comment 13

[Lukas Blakk [:lsblakk] use ?needinfo]

this test is failing on 10.6 unittests (which are running in production but hidden on the tinderbox page due to orange):

BUGNUMBER: 422348
STATUS: Proper overflow error reporting
 FAILED! [reported from test()] Proper overflow error reporting : Expected value 'InternalError: allocation size overflow', Actual value 'No Error'
REFTEST TEST-UNEXPECTED-FAIL | file:///Users/cltbld/talos-slave/mozilla-central-snowleopard-opt-u-jsreftest/build/jsreftest/tests/jsreftest.html?test=js1_5/Regress/regress-422348.js | Proper overflow error reporting Expected value 'InternalError: allocation size overflow', Actual value 'No Error'  item 1
REFTEST INFO | Loading a blank page
REFTEST INFO | Loading file:///Users/cltbld/talos-slave/mozilla-central-snowleopard-opt-u-jsreftest/build/jsreftest/tests/jsreftest.html?test=js1_5/Regress/regress-424311.js
begin test: js1_5/Regress/regress-424311.js

Comment 14

[Mike Shaver (:shaver emeritus)]

This test will be an unexpected success on all 64-bit builds, I suspect.  Can you link to a tinderbox log to confirm the platform details?

Comment 15

[Bob Clary [:bc] (inactive)]

The test is marked as failing for 64bit Linux in js1_5/Regress/jstests.list

fails-if(xulRuntime.OS=="Linux"&&xulRuntime.XPCOMABI.match(/x86_64/)) script regress-422348.js

If the 10.6 box is x86_64, then removing the xulRuntime.OS part of the conditional should make this go green.

Comment 16

[Bob Clary [:bc] (inactive)]

this is still fixed. lets deal with the 10.6 64bit issues in bug 561354