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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

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

Comment 1

[Christian Holler (:decoder)]

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?

Comment 2

[Luke Wagner [:luke]]

You are correct, that code should be reporting an error.

Comment 3

[Christian Holler (:decoder)]

(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.

Comment 4

[Christian Holler (:decoder)]

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?

Comment 5

[Christian Holler (:decoder)]

Attachment: WIP Patch

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! :)

Comment 6

[Christian Holler (:decoder)]

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.

Comment 7

[Christian Holler (:decoder)]

Attachment: Updated patch

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.

Comment 8

[Brian Hackett [Laid off!]]

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.

Comment 9

[Christian Holler (:decoder)]

Attachment: Updated patch

Removed braces and reported OOM already in toDictionaryMode :)

Comment 10

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

Comment on attachment 604644 [details] [diff] [review]
Updated patch

http://hg.mozilla.org/integration/mozilla-inbound/rev/248590650201

Comment 11

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

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.

Comment 12

[Christian Holler (:decoder)]

(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.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Relanded.
https://hg.mozilla.org/integration/mozilla-inbound/rev/c6b04bbe6890

Comment 14

[Daniel Holbert [:dholbert]]

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