Closed Bug 727326 Opened 9 years ago Closed 9 years ago

JS OOM Testing: Assertion failure: cx->isExceptionPending() || cx->runtime->hadOutOfMemory, at methodjit/Compiler.cpp:1009

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla13

People

(Reporter: decoder, Assigned: decoder)

Details

(Keywords: assertion, testcase, Whiteboard: js-triage-needed)

Attachments

(1 file, 2 obsolete files)

The following command asserts on mozilla-central revision d45c7d7b0079:

js -m -n -a -A 1327 -f js/src/tests/shell.js -f js/src/tests/shell.js
I do have an idea what the problem here is:

In methodjit/FrameState.cpp, we have certain OOM conditions (mostly LifoAllocs) that are handled by returning NULL, but js_ReportOutOfMemory is never called. It seems though that the caller expects this to happen (according to the assert).

One example is in FrameState::computeAllocation where it says:

>    RegisterAllocation *alloc = cx->typeLifoAlloc().new_<RegisterAllocation>(false);
>    if (!alloc)
>        return NULL;

Should we call js_ReportOutOfMemory(cx) before returning here?
You are correct, that code should be reporting an error.
(In reply to Luke Wagner [:luke] from comment #2)
> You are correct, that code should be reporting an error.

Thanks. I'll come up with a patch tomorrow that fixes all of these occurrences. It's not limited to FrameState.cpp, but also in Compiler.cpp. Fortunately, these are easy to recognize.
I already fixed some of the causes for this assertion, but I'm not entirely sure at which level I should fix this trace:

#0 js/src/debug64-bt2/js(+0x432771) (js_calloc at /home/decoder/LangFuzz/mozilla-central/js/src/debug64-bt2/./dist/include/js/Utility.h:159)
#1 js/src/debug64-bt2/js(+0x4540f9) (JSRuntime::calloc_(unsigned long, JSContext*) at /home/decoder/LangFuzz/mozilla-central/js/src/jscntxt.h:620)
#2 js/src/debug64-bt2/js(+0x5a02f2) (js::PropertyTable::init(JSRuntime*, js::Shape*) at /home/decoder/LangFuzz/mozilla-central/js/src/jsscope.cpp:88)
#3 js/src/debug64-bt2/js(+0x5a075b) (js::Shape::hashify(JSContext*) at /home/decoder/LangFuzz/mozilla-central/js/src/jsscope.cpp:161)
#4 js/src/debug64-bt2/js(+0x5a15b7) (JSObject::toDictionaryMode(JSContext*) at /home/decoder/LangFuzz/mozilla-central/js/src/jsscope.cpp:469)
#5 js/src/debug64-bt2/js(+0x5a2092) (JSObject::addPropertyInternal(JSContext*, jsid, int (*)(JSContext*, JSObject*, jsid, JS::Value*), int (*)(JSContext*, JSObject*, jsid, int, JS::Value*), unsigned int, unsigned int, unsigned int, int, js::Shape**, bool) at /home/decoder/LangFuzz/mozilla-central/js/src/jsscope.cpp:625)
#6 js/src/debug64-bt2/js(+0x5a1ec2) (JSObject::addProperty(JSContext*, jsid, int (*)(JSContext*, JSObject*, jsid, JS::Value*), int (*)(JSContext*, JSObject*, jsid, int, JS::Value*), unsigned int, unsigned int, unsigned int, int, bool) at /home/decoder/LangFuzz/mozilla-central/js/src/jsscope.cpp:602)
#7 js/src/debug64-bt2/js(+0x46ef26) (JSObject::addDataProperty(JSContext*, jsid, unsigned int, unsigned int) at /home/decoder/LangFuzz/mozilla-central/js/src/debug64-bt2/../jsobj.h:1049)
#8 js/src/debug64-bt2/js(+0x4724a5) (js::DefineConstructorAndPrototype(JSContext*, js::GlobalObject*, JSProtoKey, JSObject*, JSObject*) at /home/decoder/LangFuzz/mozilla-central/js/src/debug64-bt2/../jsobjinlines.h:1886)
#9 js/src/debug64-bt2/js(+0x469316) (js_InitArrayClass(JSContext*, JSObject*) at /home/decoder/LangFuzz/mozilla-central/js/src/jsarray.cpp:3726)
#10 js/src/debug64-bt2/js(+0x4f93a7) (js::GlobalObject::getOrCreateArrayPrototype(JSContext*) at /home/decoder/LangFuzz/mozilla-central/js/src/vm/GlobalObject.h:245)
#11 js/src/debug64-bt2/js(+0x725ae2) (GetDenseArrayShape at /home/decoder/LangFuzz/mozilla-central/js/src/methodjit/FastOps.cpp:1521)
#12 js/src/debug64-bt2/js(+0x72905b) (js::mjit::Compiler::jsop_getelem() at /home/decoder/LangFuzz/mozilla-central/js/src/methodjit/FastOps.cpp:2227)
#13 js/src/debug64-bt2/js(+0x6baa98) (js::mjit::Compiler::generateMethod() at /home/decoder/LangFuzz/mozilla-central/js/src/methodjit/Compiler.cpp:2560)
#14 js/src/debug64-bt2/js(+0x6b065e) (js::mjit::Compiler::performCompilation() at /home/decoder/LangFuzz/mozilla-central/js/src/methodjit/Compiler.cpp:550)
#15 js/src/debug64-bt2/js(+0x6af28f) (js::mjit::Compiler::compile() at /home/decoder/LangFuzz/mozilla-central/js/src/methodjit/Compiler.cpp:150)
#16 js/src/debug64-bt2/js(+0x6b2613) (js::mjit::CanMethodJIT(JSContext*, JSScript*, unsigned char*, bool, js::mjit::CompileRequest) at /home/decoder/LangFuzz/mozilla-central/js/src/methodjit/Compiler.cpp:997)


At some level here, I must call js_ReportOutOfMemory, where should that usually happen? In js::PropertyTable::init there is a comment:

>    /*
>     * Use rt->calloc_ for memory accounting and overpressure handling
>     * without OOM reporting. See PropertyTable::change.
>     */
>    entries = (Shape **) rt->calloc_(sizeOfEntries(JS_BIT(sizeLog2)));
>    if (!entries)
>        return false;

So I assume that isn't the right place?
Attached patch WIP Patch (obsolete) — Splinter Review
First patch which fixes some of the "hadOutOfMemory" asserts I got (I only modified those locations that indeed triggered that assertion for me, to avoid reporting OOM twice).

However, the bug from my previous comment is not in there yet, as I don't know at which level I should report OOM. Shall I include this in this patch, and if so, where? Is there a general rule of thumb for the future? Thanks! :)
Assignee: general → choller
Status: NEW → ASSIGNED
Attachment #603251 - Flags: feedback?(bhackett1024)
I looked through the trace in comment 4 further and also at the bug which added the "without OOM reporting" comment there. The msg said:

> summary:     PropertyTable::{init,change} should use js_calloc/js_free, and 
> JSObject::addPropertyInternal should report OOM on change failure (606880, r=anygregor).

However, JSObject::addPropertyInternal does not report OOM, but it seems like a good place. All functions below that in the trace don't necessarily need to succeed (if I understand them correctly), while the addPropertyInternal method needs the dictionary mode switch to succeed.
Attached patch Updated patch (obsolete) — Splinter Review
Updated the patch as described in comment 6. I hope I have caught all of the missing occurrences now that account for this assert. If not, then we should open a followup bug.
Attachment #603251 - Attachment is obsolete: true
Attachment #604177 - Flags: review?(bhackett1024)
Attachment #603251 - Flags: feedback?(bhackett1024)
Comment on attachment 604177 [details] [diff] [review]
Updated patch

Review of attachment 604177 [details] [diff] [review]:
-----------------------------------------------------------------

Can you attach a patch with the toDictionaryMode stuff fixed?  Looks good otherwise.

::: js/src/jsarray.cpp
@@ -3850,2 @@
>          THROWV(NULL);
>      }

You can remove the braces here now.

::: js/src/jsscope.cpp
@@ +622,5 @@
>          JS_ASSERT_IF(!allowDictionary, stableSlot);
>          if (allowDictionary &&
>              (!stableSlot || lastProperty()->entryCount() >= PropertyTree::MAX_HEIGHT)) {
> +            if (!toDictionaryMode(cx)) {
> +                js_ReportOutOfMemory(cx);

I think this reporting should happen inside JSObject::toDictionaryMode instead.  There are other callers of toDictionaryMode not fixed by this patch.
Attachment #604177 - Flags: review?(bhackett1024)
Attached patch Updated patchSplinter Review
Removed braces and reported OOM already in toDictionaryMode :)
Attachment #604177 - Attachment is obsolete: true
Attachment #604644 - Flags: review?(bhackett1024)
Attachment #604644 - Flags: review?(bhackett1024) → review+
Attachment #604644 - Flags: checkin?(gary)
Backed out - either this patch or the patch in bug 733493 set mozilla-inbound tbpl on fire.

http://hg.mozilla.org/integration/mozilla-inbound/rev/7fd16b1a4f50

Please submit the patch to the tryserver first.
(In reply to Gary Kwong [:gkw, :nth10sd] from comment #11)
> Backed out - either this patch or the patch in bug 733493 set
> mozilla-inbound tbpl on fire.
> 

Failure is unrelated to this patch and can be relanded.
Merged to m-c...
 the initial cset:   https://hg.mozilla.org/mozilla-central/rev/248590650201
 and backout:        https://hg.mozilla.org/mozilla-central/rev/7fd16b1a4f50
 and re-landed cset: https://hg.mozilla.org/mozilla-central/rev/c6b04bbe6890
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.