Closed Bug 624094 Opened 9 years ago Closed 1 year ago

[meta] A test for OOM conditions in the shell

Categories

(Core :: JavaScript Engine, defect)

5 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla2.0

People

(Reporter: paul.biggar, Unassigned)

References

(Blocks 1 open bug)

Details

(Keywords: meta)

Attachments

(1 file, 3 obsolete files)

Attached patch Add OOM testing (obsolete) — Splinter Review
(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.
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
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.
(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.
The last patch was broken, this one actually finds tons of errors. Discussion to follow.
Attachment #502193 - Attachment is obsolete: true
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)
Attached patch Test OOM from a build dir (obsolete) — Splinter Review
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
Attachment #504545 - Attachment is obsolete: true
Depends on: 626492
Blocks: 626494
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
Attachment #504552 - Attachment is obsolete: true
Depends on: 626526
Depends on: 626531
(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().
Depends on: 626706
No longer blocks: 626494
Depends on: 626494
Depends on: 628332
Depends on: 628446
Assignee: general → pbiggar
OS: Mac OS X → All
Hardware: x86 → All
Blocks: 636940
Making this meta to track OOMs.
Summary: A test for OOM conditions in the shell → [meta] A test for OOM conditions in the shell
Depends on: 642327
Duplicate of this bug: 383932
Duplicate of this bug: 626706
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.
Whiteboard: [mentor=pbiggar@mozilla.com]
Target Milestone: --- → mozilla2.0
Version: Trunk → 5 Branch
Duplicate of this bug: 626706
Depends on: 660610
Depends on: 660630
Depends on: 660638
Depends on: 660668
Depends on: 660670
Depends on: 660681
Assignee: paul.biggar → general
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.
Depends on: 745766
Depends on: 745768
Depends on: 745778
Depends on: 745786
Depends on: 745787
Depends on: 745794
Depends on: 745798
Depends on: 745802
Depends on: 745806
Depends on: 745807
Depends on: 745813
Depends on: 745815
Depends on: 745819
Depends on: 727334
Depends on: 756581
Depends on: 756582
Depends on: 756583
Depends on: 756600
Depends on: 756605
Depends on: 756609
Depends on: 756610
Depends on: 756611
Depends on: 756612
Depends on: 756613
Depends on: 756614
Depends on: 756615
Depends on: 756616
Depends on: 756617
Depends on: 756618
Depends on: 756619
Depends on: 756621
Depends on: 756622
Depends on: 756623
Depends on: 756624
Depends on: 756626
Depends on: 756627
Depends on: 756628
Depends on: 756629
Depends on: 756630
Depends on: 756632
Depends on: 756771
Depends on: 756772
Depends on: 756773
Depends on: 756774
Depends on: 777182
Depends on: 777183
Depends on: 777184
Depends on: 777186
Terrence, can you comment on whether this is still a good first bug and, if so, who could mentor it?
Flags: needinfo?(terrence)
Whiteboard: [mentor=pbiggar@mozilla.com]
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.
(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.
Flags: needinfo?(terrence)
Assignee: general → nobody
: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.
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.