Closed Bug 840012 Opened 10 years ago Closed 10 years ago

IonMonkey: Assertion failure: obj, at ../dist/include/js/Value.h:482 or Crash on Heap near [@ EnterIon] with OOM


(Core :: JavaScript Engine, defect)

Not set





(Reporter: decoder, Unassigned)



(Keywords: assertion, crash, testcase, Whiteboard: [jsbugmon:update])

Crash Data


(1 file, 1 obsolete file)

The following testcase asserts on mozilla-central revision a46bc920998d (run with --ion-eager):

evaluate("gcparam(\"maxBytes\", gcparam(\"gcBytes\") + 4*1024);");
function testDontEnum(F) { \
  function test() {\
    typeof (new test(\"1\")) != 'function'\
} \
var list = [];\
for (i in list)\
  var F = this[list[i]];\
actual = testDontEnum(F);\
Pretty stable out-of-memory crash/assert, trying to bisect it.
Crash Signature: [@ EnterIon]
Keywords: crash
Whiteboard: [jsbugmon:update,bisect]
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   95790:b863ef9946b8
user:        Luke Wagner
date:        Thu Feb 23 13:59:10 2012 -0800
summary:     Bug 659577 - Don't alias stack variables (r=bhackett)

This iteration took 55.811 seconds to run.
Ccing luke and bhackett based on comment 2. I guess this is not actually the regressing changeset?
I'm not sure whether bug 659577 caused the problem or just perturbed allocation behavior.  Regardless, this just seems to be an innocuous assertion: the epilogue is copying the 'contructorThis' value into the return value which asserts that 'constructorThis' is an object.  However, because of the OOM 'constructorThis' value is still 'undefined'.  This fix, I think, is for StackFrame::epilogue to just copy formals()[-1] directly.
(In reply to Luke Wagner [:luke] from comment #4)
> Regardless, this just seems to be an innocuous assertion:

But the test crashes in an opt build, so I guess it can't just be the assertion, right?
Oh, I didn't see any mention of opt crash.
See the subject and crash signature :) The crash is in jitted code:

Program received signal SIGSEGV, Segmentation fault.
0xf7fcb636 in ?? ()
(gdb) bt 4
#0  0xf7fcb636 in ?? ()
#1  0x083221ff in EnterIon (cx=0x0, fp=0xf7426f60, jitcode=0xf7fcb610) at /srv/repos/mozilla-central/js/src/ion/Ion.cpp:1731
#2  0x080fbb0c in js::Interpret (cx=0x85b0340, entryFrame=0xf7698088, interpMode=js::JSINTERP_NORMAL) at /srv/repos/mozilla-central/js/src/jsinterp.cpp:2376
#3  0x080fe094 in js::RunScript (cx=0x85b0340, fp=0xf7698088) at /srv/repos/mozilla-central/js/src/jsinterp.cpp:324
(More stack frames follow...)
(gdb) x /i $pc
=> 0xf7fcb636:  mov    0x4(%ecx),%ecx
(gdb) info reg ecx
ecx            0x0      0
Attached patch maybe fix (obsolete) — Splinter Review
I couldn't reproduce the crash in an opt build, but this patch does fix the assertion in the obvious way.  Decoder, could you test whether this fixes the opt crash?
Attachment #715300 - Flags: feedback?(choller)
Unfortunately, this didn't fix anything for me. With the patch I still get the assert, and in the opt-build a crash.
Attachment #715300 - Flags: feedback?(choller) → feedback-
Blocks: 806344
Whiteboard: [jsbugmon:update] → [jsbugmon:]
JSBugMon: Cannot process bug: Unknown exception (check manually)
Whiteboard: [jsbugmon:] → [jsbugmon:update]
Can we get this issue fixed? It is also causing crashes in Baseline on the heap, and these are hard to triage.
Perhaps Luke might know - he had a "maybe fix".
Flags: needinfo?(luke)
Attached patch more OOM fixesSplinter Review
Ah, I can reproduce now.  Lots of OOM corner cases.

decoder/gary: I noticed that, while the test added in this patch does trigger the OOM crash, the // |jit-test| error:out of memory line in the manifest causes us to report "tests pass" (because "out of memory" is printed before crashing).  It seems like we should fix the harness to always fail for crashes.
Attachment #715300 - Attachment is obsolete: true
Attachment #738245 - Flags: review?(hv1989)
Flags: needinfo?(luke)
Comment on attachment 738245 [details] [diff] [review]
more OOM fixes

Review of attachment 738245 [details] [diff] [review]:

Nice catch!

::: js/src/vm/Stack.cpp
@@ +406,5 @@
>      if (cx->compartment->debugMode())
>          DebugScopes::onPopCall(this, cx);
>      if (isConstructing() && thisValue().isObject() && returnValue().isPrimitive())
> +        setReturnValue(formals()[-1]);

As far as I see, this isn't a correctness issue?! Am I correct? I must say the previous was better readable, because it hides the fact that "this" is at -1 place...
Attachment #738245 - Flags: review?(hv1989) → review+
(In reply to Hannes Verschore [:h4writer] from comment #14)
> As far as I see, this isn't a correctness issue?!

I don't quite understand that question; I think this was only a problem if we OOM while constructing 'this'.

> I must say the previous was better readable, because it hides the fact that
> "this" is at -1 place...

True; I meant to add a comment, so I'll do that.
Hm, that looks like the OOM issue I fixed in bug 858097 (see comment 2 there).
Hah, I hadn't even noticed your change in the rebase.  I'll remove that change from my patch, then.

Did you intend to name this test testBug0012.js?
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Duplicate of this bug: 806344
Duplicate of this bug: 858209
Duplicate of this bug: 822223
You need to log in before you can comment on or make changes to this bug.