624094 - [meta] A test for OOM conditions in the shell

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Paul Biggar]

Attachment: Add OOM testing

(It occurred to me when I was about 90% of the way through that it would be better to do this by static analysis. Do we have anything like that?).

The attached patch makes js_malloc and friends return NULL after X allocations (set using the -A parameter to the shell). I am currently running the test in a loop [for X in range(50000]). So far no failures for X < 384.

Clearly, this isn't appropriate for inclusion in SpiderMonkey, so just putting here to document it.

Comment 1

[Brendan Eich [:brendan]]

Please see bug 383932 and take the review request away from despicable me (who has ignored it for way too long). Dup if appropriate. Thanks,

/be

Comment 2

[Gavin Reaney]

As I recall, the OOM handling in the JS engine is very good so you shouldn't expect to see a lot of failures. The first several hundred or thousand allocations will be for common code that gets run all the time so it is probably well tested already.

Paul's code is a lot simpler than mine so if it catches most of the same problems then it's the way to go. I can't remember all of my reasons for doing things the way I did, but possibly it was to ensure that we also catch direct calls to malloc (not everyone uses JS_malloc), and also to catch memory leaks and corruption.

Since you can presumably use valgrind or other tools to detect these other issues it may be better to keep things simple.

Comment 3

[Paul Biggar]

(In reply to comment #2)
> The first several hundred or thousand
> allocations will be for common code that gets run all the time so it is
> probably well tested already.

Right. I haven't hit anything for |a < 17000| so maybe I should check this.


> Paul's code is a lot simpler than mine so if it catches most of the same
> problems then it's the way to go. I can't remember all of my reasons for doing
> things the way I did, but possibly it was to ensure that we also catch direct
> calls to malloc (not everyone uses JS_malloc), and also to catch memory leaks
> and corruption.

Was yours intended for inclusion in the source tree? Mine is certainly unsuitable.

I think we shouldn't be calling malloc ever, is that right? If so, that can be handled by bug 624878.

Comment 4

[Paul Biggar]

Attachment: Run tests with increasing OOM counts

The last patch was broken, this one actually finds tons of errors. Discussion to follow.

Comment 5

[Paul Biggar]

Here's sample output from jit-test/tests/arguments/args-mochi-2.js, which is the first file I ran. There are 13 errors:

 - 9 handle OOM, then segfault anyway
 - 5 have assertion errors
 - 3 are in Yarr
 - 2 give more than 1 OOM message


Output:

Running jit-test/tests/arguments/args-createontrace.js, 119/213
/Users/pbiggar/work/mozilla/bug623859/js/src/build_add_testing_DBG.OBJ/shell/js -A 119 -m -j -p -e "const platform='darwin'; const libdir='/Users/pbiggar/work/mozilla/bug623859/js/src/jit-test/lib/';" -f /Users/pbiggar/work/mozilla/bug623859/js/src/jit-test/lib/prolog.js -f jit-test/tests/arguments/args-createontrace.js
Found problem('', 'out of memory\nAssertion failure: JSID_BITS(id) == JSID_TYPE_VOID, at ../jsapi.h:452\n', -11)


Running jit-test/tests/arguments/args-createontrace.js, 121/213
/Users/pbiggar/work/mozilla/bug623859/js/src/build_add_testing_DBG.OBJ/shell/js -A 121 -m -j -p -e "const platform='darwin'; const libdir='/Users/pbiggar/work/mozilla/bug623859/js/src/jit-test/lib/';" -f /Users/pbiggar/work/mozilla/bug623859/js/src/jit-test/lib/prolog.js -f jit-test/tests/arguments/args-createontrace.js
Found problem('', 'Assertion failure: minimumInputSize != UINT_MAX, at ../yarr/yarr/RegexCompiler.cpp:576\n', -11)
No OOM message


Running jit-test/tests/arguments/args-createontrace.js, 129/213
/Users/pbiggar/work/mozilla/bug623859/js/src/build_add_testing_DBG.OBJ/shell/js -A 129 -m -j -p -e "const platform='darwin'; const libdir='/Users/pbiggar/work/mozilla/bug623859/js/src/jit-test/lib/';" -f /Users/pbiggar/work/mozilla/bug623859/js/src/jit-test/lib/prolog.js -f jit-test/tests/arguments/args-createontrace.js
Found problem('', 'Assertion failure: !start->table, at ../jsscope.h:874\n', -11)
No OOM message


Running jit-test/tests/arguments/args-createontrace.js, 132/213
/Users/pbiggar/work/mozilla/bug623859/js/src/build_add_testing_DBG.OBJ/shell/js -A 132 -m -j -p -e "const platform='darwin'; const libdir='/Users/pbiggar/work/mozilla/bug623859/js/src/jit-test/lib/';" -f /Users/pbiggar/work/mozilla/bug623859/js/src/jit-test/lib/prolog.js -f jit-test/tests/arguments/args-createontrace.js
Found problem('', 'Assertion failure: minimumInputSize != UINT_MAX, at ../yarr/yarr/RegexCompiler.cpp:576\n', -11)
No OOM message


Running jit-test/tests/arguments/args-createontrace.js, 135/213
/Users/pbiggar/work/mozilla/bug623859/js/src/build_add_testing_DBG.OBJ/shell/js -A 135 -m -j -p -e "const platform='darwin'; const libdir='/Users/pbiggar/work/mozilla/bug623859/js/src/jit-test/lib/';" -f /Users/pbiggar/work/mozilla/bug623859/js/src/jit-test/lib/prolog.js -f jit-test/tests/arguments/args-createontrace.js
Found problem('', 'Assertion failure: minimumInputSize != UINT_MAX, at ../yarr/yarr/RegexCompiler.cpp:576\n', -11)
No OOM message


Running jit-test/tests/arguments/args-createontrace.js, 141/213
/Users/pbiggar/work/mozilla/bug623859/js/src/build_add_testing_DBG.OBJ/shell/js -A 141 -m -j -p -e "const platform='darwin'; const libdir='/Users/pbiggar/work/mozilla/bug623859/js/src/jit-test/lib/';" -f /Users/pbiggar/work/mozilla/bug623859/js/src/jit-test/lib/prolog.js -f jit-test/tests/arguments/args-createontrace.js
Found problem('', 'out of memory\n', -11)


Running jit-test/tests/arguments/args-createontrace.js, 163/213
/Users/pbiggar/work/mozilla/bug623859/js/src/build_add_testing_DBG.OBJ/shell/js -A 163 -m -j -p -e "const platform='darwin'; const libdir='/Users/pbiggar/work/mozilla/bug623859/js/src/jit-test/lib/';" -f /Users/pbiggar/work/mozilla/bug623859/js/src/jit-test/lib/prolog.js -f jit-test/tests/arguments/args-createontrace.js
Found problem('', 'out of memory\n', -11)


Running jit-test/tests/arguments/args-createontrace.js, 186/213
/Users/pbiggar/work/mozilla/bug623859/js/src/build_add_testing_DBG.OBJ/shell/js -A 186 -m -j -p -e "const platform='darwin'; const libdir='/Users/pbiggar/work/mozilla/bug623859/js/src/jit-test/lib/';" -f /Users/pbiggar/work/mozilla/bug623859/js/src/jit-test/lib/prolog.js -f jit-test/tests/arguments/args-createontrace.js
Found problem('', 'out of memory\n', -11)


Running jit-test/tests/arguments/args-createontrace.js, 200/213
/Users/pbiggar/work/mozilla/bug623859/js/src/build_add_testing_DBG.OBJ/shell/js -A 200 -m -j -p -e "const platform='darwin'; const libdir='/Users/pbiggar/work/mozilla/bug623859/js/src/jit-test/lib/';" -f /Users/pbiggar/work/mozilla/bug623859/js/src/jit-test/lib/prolog.js -f jit-test/tests/arguments/args-createontrace.js
Found problem('', 'out of memory\nout of memory\n', -11)


Running jit-test/tests/arguments/args-createontrace.js, 203/213
/Users/pbiggar/work/mozilla/bug623859/js/src/build_add_testing_DBG.OBJ/shell/js -A 203 -m -j -p -e "const platform='darwin'; const libdir='/Users/pbiggar/work/mozilla/bug623859/js/src/jit-test/lib/';" -f /Users/pbiggar/work/mozilla/bug623859/js/src/jit-test/lib/prolog.js -f jit-test/tests/arguments/args-createontrace.js
Found problem('', 'out of memory\n', -11)


Running jit-test/tests/arguments/args-createontrace.js, 206/213
/Users/pbiggar/work/mozilla/bug623859/js/src/build_add_testing_DBG.OBJ/shell/js -A 206 -m -j -p -e "const platform='darwin'; const libdir='/Users/pbiggar/work/mozilla/bug623859/js/src/jit-test/lib/';" -f /Users/pbiggar/work/mozilla/bug623859/js/src/jit-test/lib/prolog.js -f jit-test/tests/arguments/args-createontrace.js
Found problem('', 'out of memory\nout of memory\n', -11)


Running jit-test/tests/arguments/args-createontrace.js, 209/213
/Users/pbiggar/work/mozilla/bug623859/js/src/build_add_testing_DBG.OBJ/shell/js -A 209 -m -j -p -e "const platform='darwin'; const libdir='/Users/pbiggar/work/mozilla/bug623859/js/src/jit-test/lib/';" -f /Users/pbiggar/work/mozilla/bug623859/js/src/jit-test/lib/prolog.js -f jit-test/tests/arguments/args-createontrace.js
Found problem('', 'out of memory\n', -11)


Running jit-test/tests/arguments/args-createontrace.js, 210/213
/Users/pbiggar/work/mozilla/bug623859/js/src/build_add_testing_DBG.OBJ/shell/js -A 210 -m -j -p -e "const platform='darwin'; const libdir='/Users/pbiggar/work/mozilla/bug623859/js/src/jit-test/lib/';" -f /Users/pbiggar/work/mozilla/bug623859/js/src/jit-test/lib/prolog.js -f jit-test/tests/arguments/args-createontrace.js
Found problem('', 'out of memory\n', -11)

Comment 6

[Paul Biggar]

Attachment: Test OOM from a build dir

This takes my build dir information out of the test, to make it easier to report bugs and for others to run. Run it by:

 $ cd build_dir
 $ ../OOM.py

Comment 7

[Paul Biggar]

Attachment: With helpful diagnostics and stack traces and blacklists

This version:

 - prints an error message I can paste into bugzilla
 - adds stack traces (via valgrind)
 - blacklists errors we've seen before
 - tries to figure out what's wrong

Comment 8

[Nicholas Nethercote [inactive]]

(In reply to comment #3)
> 
> I think we shouldn't be calling malloc ever, is that right? If so, that can be
> handled by bug 624878.

Nope:  we shouldn't ever call vanilla |operator new|, and that's what bug 624878 protects against.

We call malloc() directly from js_malloc(), unless JS_USE_CUSTOM_ALLOCATOR is defined -- that's a way for embeddings to use a custom allocator instead of malloc().

Comment 9

[Paul Biggar]

Making this meta to track OOMs.

Comment 12

[Paul Biggar]

OOM errors are easy to track down (given supplied script) and easy to fix. Also, they give a wide exposure to the engine. Simply take the script config/find_OOM_errors.py, then look in OOM.log to find errors that need to be fixed.

Comment 14

[Christian Holler (:decoder)]

Last week I've been working on a script that does JS shell OOM testing using -A. I only found the find_OOM_errors.py script afterwards but that only seems to support jit-test anyhow.

My script does a binary search on the required number of allocations and then keeps decreasing the number till reaching min (~100). It found quite a few failures so far, so I think we should add it to our tests.

We also should add the JS_OOM_POSSIBLY_FAIL() macro to LifoAlloc and NewGCThing to catch unchecked failures there.

I'm willing to take this if nobody else is working on it.

Comment 15

[Till Schneidereit [:till]]

Terrence, can you comment on whether this is still a good first bug and, if so, who could mentor it?

Comment 16

[Christian Holler (:decoder)]

I did a lot of work with the OOM testing and I would be willing to help mentoring this. There is one big problem here, and that is that OOM bugs don't tend to be fixed, except if they are security-related, at least on trunk. On IonMonkey/Baseline, OOM bugs we're fixed pretty quickly until they landed.

Comment 17

[Terrence Cole [:terrence]]

(In reply to Till Schneidereit [:till] from comment #15)
> Terrence, can you comment on whether this is still a good first bug and, if
> so, who could mentor it?

Well, as always, it depends. As Christian mentions, we don't usually get around to fixing OOM bugs. It's not that we don't care, it's just that (1) they are extremely unlikely to ever hit in practice, (2) they are basically never exploitable, and (3) there are tons and tons of them because there are many error pathways and very few tests of those paths. The combination of these 3 things means that OOM bugs generally are the last thing on our todo list and we have a /very/ long todo list. However, if I ever do have, I would totally fix these bugs.

Does that make these bugs good candidates for the [good first bug] tag? I think it depends on the person. For someone who knows how to code in C/C++ in an open-source environment already and just needs to get familiar with our code base, these would be excellent bugs to start with. My own first bug was actually an OOM bug in TM. However, if the person working on these bugs is going to require more than a quick review and a couple of procedural questions, I would prefer to help them with something that is just as simple, but higher impact. An example would be the CallArgs conversion in bug 845478.

I'm sorry I don't have a more helpful answer here.

Comment 18

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

:decoder has a more updated OOM meta bug 912928 and even that has only a few open bugs left. Since this one has had no new ones in 6+ years, let's declare this one FIXED and move on.