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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

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


gcPreserveCode();
evaluate("gcparam(\"maxBytes\", gcparam(\"gcBytes\") + 4*1024);");
evaluate("\
function testDontEnum(F) { \
  function test() {\
    typeof (new test(\"1\")) != 'function'\
  }\
  test();\
} \
var list = [];\
for (i in list)\
  var F = this[list[i]];\
actual = testDontEnum(F);\
");

Comment 1

[Christian Holler (:decoder)]

Pretty stable out-of-memory crash/assert, trying to bisect it.

Comment 2

[Christian Holler (:decoder)]

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.

Comment 3

[Christian Holler (:decoder)]

Ccing luke and bhackett based on comment 2. I guess this is not actually the regressing changeset?

Comment 4

[Luke Wagner [:luke]]

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.

Comment 5

[Christian Holler (:decoder)]

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

Comment 6

[Luke Wagner [:luke]]

Oh, I didn't see any mention of opt crash.

Comment 7

[Christian Holler (:decoder)]

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

Comment 8

[Luke Wagner [:luke]]

Attachment: maybe fix

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?

Comment 9

[Christian Holler (:decoder)]

Unfortunately, this didn't fix anything for me. With the patch I still get the assert, and in the opt-build a crash.

Comment 10

[Christian Holler (:decoder)]

JSBugMon: Cannot process bug: Unknown exception (check manually)

Comment 11

[Christian Holler (:decoder)]

Can we get this issue fixed? It is also causing crashes in Baseline on the heap, and these are hard to triage.

Comment 12

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

Perhaps Luke might know - he had a "maybe fix".

Comment 13

[Luke Wagner [:luke]]

Attachment: more OOM fixes

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 jit_tests.py harness to always fail for crashes.

Comment 14

[Hannes Verschore [:h4writer]]

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

Comment 15

[Luke Wagner [:luke]]

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

Comment 16

[Jan de Mooij [:jandem]]

Hm, that looks like the OOM issue I fixed in bug 858097 (see comment 2 there).

Comment 17

[Luke Wagner [:luke]]

Hah, I hadn't even noticed your change in the rebase.  I'll remove that change from my patch, then.

Comment 18

[Luke Wagner [:luke]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/64198b55d1ae

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/64198b55d1ae

Did you intend to name this test testBug0012.js?

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Fixup for the test name:
https://hg.mozilla.org/integration/mozilla-inbound/rev/16d1cff60244

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/16d1cff60244