Status

()

enhancement
RESOLVED FIXED
11 years ago
9 years ago

People

(Reporter: igor, Assigned: igor)

Tracking

Trunk
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

v2
14.08 KB, patch
igor
: review+
shaver
: approval1.9+
Details | Diff | Splinter Review
Assignee

Description

11 years ago
+++ 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: ..."
Assignee

Updated

11 years ago
Summary: Proper reporting overflow errors → Proper overflow error reporting
Assignee

Comment 1

11 years ago
Posted patch v1 (obsolete) — Splinter Review
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
Attachment #308875 - Flags: review?(brendan)

Comment 2

11 years ago
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?
You can't catch OOM on the branch; will that still work?

Comment 4

11 years ago
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.
Assignee

Comment 5

11 years ago
(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 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!
Attachment #308875 - Flags: review?(brendan) → review+
Assignee

Comment 7

11 years ago
Posted patch v2Splinter Review
The new version renames the reporting function to js_ReportAllocationOverflow (which is indeed a better name) and exposes the functionality as a public API .
Attachment #308875 - Attachment is obsolete: true
Attachment #308903 - Flags: review+
Attachment #308903 - Flags: approval1.9?
Comment on attachment 308903 [details] [diff] [review]
v2

a=shaver for making class of errors handleable by Firebug and otherwise.
Attachment #308903 - Flags: approval1.9? → approval1.9+
Assignee

Comment 9

11 years ago
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
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Comment 10

11 years ago
/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

11 years ago
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

11 years ago
verified linux|mac|windows

/cvsroot/mozilla/js/tests/js1_5/Regress/regress-422348.js,v  <--  regress-422348.js
initial revision: 1.1
Status: RESOLVED → VERIFIED
Flags: in-testsuite+
Flags: in-litmus-
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
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
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

9 years ago
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

9 years ago
this is still fixed. lets deal with the 10.6 64bit issues in bug 561354
Status: REOPENED → RESOLVED
Last Resolved: 11 years ago9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.