523947 - TM: Crash [@ JS_CallTracer]

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

Attachment: stacktraces

Function("\
  (function foo(a, c) {\
  if (a.length == c) {\
    return ((function() { z }))\
  }\
  b = foo(a, c + 1);\
  return ((function foo2(d) {\
    return d.length == 0 ? 0 : d[0] + foo2(d.slice(1))\
  })([b, b, , 0x5a827999, /x/, , b, /x/]))\
})([,,,,,,,,,,,,0], 0)\
")()

crashes js opt shell with -j on TM tip at JS_CallTracer near null and crashes js debug shell with -j on TM tip at JS_CallTracer at a scary 0xdadadfec location (turning security-sensitive because of this).

Nominating blocking-1.9.2? because bug 523370 is labelled as status1.9.2beta1-fixed.

autoBisect shows this is probably related to bug 523370:

The first bad revision is:
changeset:   34047:a389613bd931
user:        Igor Bukanov
date:        Thu Oct 22 01:03:56 2009 +0400
summary:     bug 523370 - fixing bogus OOM with empty double free lists. r=dmandelin

Comment 1

[Jesse Ruderman]

Hitting this a lot.

Comment 2

[David Mandelin [:dmandelin]]

Weird. Does this mean starting a GC from the recorder tends to make us crash? Could it be that the GC thinks a certain frame stack slot has data, based on the PC, but that the interp opcode hasn't run yet so that slot contains garbage? Or something like that?

Comment 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: fix

since bug 505315 didn't land on 1.9.2, this doesn't need to block. I'm not sure why the other bug is marked beta1-fixed since it never landed on 1.9.2, but maybe the flags don't work in the way I am thinking.

Comment 5

[Igor Bukanov]

Comment on attachment 408455 [details] [diff] [review]
fix

>     METER(JSGCArenaStats *astats = &cx->runtime->gcStats.arenaStats[thingKind]);
>-    bool canGC = !JS_ON_TRACE(cx);
>+    bool canGC = !JS_ON_TRACE(cx) && !JS_TRACE_MONITOR(cx).useReservedObjects;

Nit: add a function like CanGC to factor out the common logic above.
Non nit: use this function also in NewGCArena where the code checks for JS_ON_TRACE(cx).

>diff --git a/js/src/jstracer.cpp b/js/src/jstracer.cpp
>+        JS_TRACE_MONITOR(cx).useReservedObjects = JS_TRUE;

Nit: rename useReservedObjects into useReservedPool or something as it is no longer used just for object allocation. Also make it bool and use true/false for its initialization.

Comment 6

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: fix

I don't think we should worry about changing/refactoring much here. It is all temporary and will go away with conservative GC, and in the meantime I'm hitting this crash in the wild.

Comment 7

[Igor Bukanov]

Comment on attachment 408663 [details] [diff] [review]
fix

>+static inline bool
>+CanGC(JSContext *cx)
>+{
>+    return !JS_ON_TRACE(cx) && !JS_TRACE_MONITOR(cx).useReservedPool;
>+}

The patch does not change NewGCArena to use CanGC, not JS_ON_TRACE, right before the js_TriggerGC call. Surely the conservative scanner will allow to resolve this, but an explicit call like CanGC will allow to trivially remove that code later without missing anything.

Comment 9

[David Anderson [:dvander] - inactive, e-mail if emergency]

I'm not as familiar with this code so I'll defer to you for the best fix :)

Comment 10

[Igor Bukanov]

Attachment: fix v2

The new version is the patch from the comment 6 with the extra change from the comment 7.

Comment 11

[Igor Bukanov]

Attachment: fix v2 for real

Here comes the right diff.

Comment 12

[David Anderson [:dvander] - inactive, e-mail if emergency]

Hrm still getting problems from running old trace-tests...


/Users/dvander/mozilla/oldmonkey/js/src/trace-test.js:5757: out of memory
Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_INVALID_ADDRESS at address: 0xdadadad8
0x0007f53b in js_Interpret (cx=0x30ab80) at jsops.cpp:1012
1012	            VALUE_TO_NUMBER(cx, -1, rval, d2);
#0  0x0007f53b in js_Interpret (cx=0x30ab80) at jsops.cpp:1012
1012	            VALUE_TO_NUMBER(cx, -1, rval, d2);
(gdb) p/x rval
$1 = 0xdadadada

Comment 13

[Andreas Gal :gal]

This is unacceptable. I will bisect and back out patches until this is fixed. I can't finish my stack scanning patch if we crash all over the place.

Comment 14

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

Bug 523370 has had erroneous 1.9.2 flags/keywords removed.

Comment 15

[Andreas Gal :gal]

Attachment: patch

Comment 16

[Andreas Gal :gal]

http://hg.mozilla.org/tracemonkey/rev/629ab1d02019

This fixes the test-case and the trace-tests crash I was seeing. The original regressor should have been backed out a week ago. It made it to m-c now. That's pretty bad.

Comment 17

[Igor Bukanov]

I should have been more attention to this bug and land the fix at least on Tuesday. Sorry about that.

Comment 18

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/629ab1d02019

Comment 19

[Christian Holler (:decoder)]

Bug in removed tracer code, setting in-testsuite- flag.