Closed
Bug 727326
Opened 11 years ago
Closed 11 years ago
JS OOM Testing: Assertion failure: cx->isExceptionPending() || cx->runtime->hadOutOfMemory, at methodjit/Compiler.cpp:1009
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla13
People
(Reporter: decoder, Assigned: decoder)
Details
(Keywords: assertion, testcase, Whiteboard: js-triage-needed)
Attachments
(1 file, 2 obsolete files)
8.08 KB,
patch
|
bhackett1024
:
review+
gkw
:
checkin+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 1•11 years ago
|
||
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•11 years ago
|
||
You are correct, that code should be reporting an error.
Assignee | ||
Comment 3•11 years ago
|
||
(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.
Assignee | ||
Comment 4•11 years ago
|
||
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?
Assignee | ||
Comment 5•11 years ago
|
||
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)
Assignee | ||
Comment 6•11 years ago
|
||
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.
Assignee | ||
Comment 7•11 years ago
|
||
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 8•11 years ago
|
||
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)
Assignee | ||
Comment 9•11 years ago
|
||
Removed braces and reported OOM already in toDictionaryMode :)
Attachment #604177 -
Attachment is obsolete: true
Attachment #604644 -
Flags: review?(bhackett1024)
Updated•11 years ago
|
Attachment #604644 -
Flags: review?(bhackett1024) → review+
Assignee | ||
Updated•11 years ago
|
Attachment #604644 -
Flags: checkin?(gary)
![]() |
||
Comment 10•11 years ago
|
||
Comment on attachment 604644 [details] [diff] [review] Updated patch http://hg.mozilla.org/integration/mozilla-inbound/rev/248590650201
Attachment #604644 -
Flags: checkin?(gary) → checkin+
![]() |
||
Comment 11•11 years ago
|
||
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.
Assignee | ||
Comment 12•11 years ago
|
||
(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•11 years ago
|
||
Relanded. https://hg.mozilla.org/integration/mozilla-inbound/rev/c6b04bbe6890
Target Milestone: --- → mozilla13
Comment 14•11 years ago
|
||
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: 11 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•