932530 - Assertion failure: thing, at gc/Marking.cpp:126 or Crash [@ tenuredZone] with OOM

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

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


function f(x) {
    "use strict";
    return delete (b);
}
const y = 1 + Math.pow(2, -52);
var diff = function (a, b) {
    f[0] = a;
    f[1] = b;
};
diff(1, y);

Comment 1

[Christian Holler (:decoder)]

Attachment: [crash-signature] Machine-readable crash signature

Comment 2

[Christian Holler (:decoder)]

Terrence, this is one of my all time favorite long standing OOM bugs that never reduced, and now it did. Can you find someone to look and fix?

Comment 3

[Terrence Cole [:terrence]]

I was not able to reproduce on the given revision with:

CC="gcc -m32" CXX="g++ -m32" ./configure --target=i686-linux-gnu --enable-optimize --enable-debug --enable-debug-symbols --enable-valgrind --enable-gczeal --enable-more-deterministic --enable-methodjit --enable-type-inference --enable-profiling --with-system-nspr --disable-tests

With gcc 4.6.3 or 4.7.3.

Comment 4

[Terrence Cole [:terrence]]

Manged to repro with --disable-threadsafe. It only reproduces in opt+debug builds.

Program received signal SIGSEGV, Segmentation fault.
0x080dc222 in CheckMarkedThing<js::types::TypeObject> (trc=trc@entry=0x937e170, thing=thing@entry=0x0) at /home/terrence/moz/branch/api_rooting/mozilla/js/src/gc/Marking.cpp:126
126	    JS_ASSERT(thing);
(gdb) bt
#0  0x080dc222 in CheckMarkedThing<js::types::TypeObject> (trc=trc@entry=0x937e170, thing=thing@entry=warning: (Internal error: pc 0x0 in read in psymtab, but not in symtab.)

0x0) at /home/terrence/moz/branch/api_rooting/mozilla/js/src/gc/Marking.cpp:126
#1  0x080e1b22 in MarkInternal<js::types::TypeObject> (trc=0x937e170, thingp=<optimized out>) at /home/terrence/moz/branch/api_rooting/mozilla/js/src/gc/Marking.cpp:174
#2  0x083d761a in js::jit::ICStub::trace (this=0x9426308, trc=0x937e170) at /home/terrence/moz/branch/api_rooting/mozilla/js/src/jit/BaselineIC.cpp:244
#3  0x08417664 in trace (trc=0x937e170, this=0x93fe618) at /home/terrence/moz/branch/api_rooting/mozilla/js/src/jit/BaselineJIT.cpp:424
#4  js::jit::BaselineScript::Trace (trc=0x937e170, script=0x93fe618) at /home/terrence/moz/branch/api_rooting/mozilla/js/src/jit/BaselineJIT.cpp:441
#5  0x08238f38 in JSScript::markChildren (this=0xf782d180, trc=0x937e170) at /home/terrence/moz/branch/api_rooting/mozilla/js/src/jsscript.cpp:2752
#6  0x080e05e7 in MarkChildren (trc=0x937e170, script=0xf782d180) at /home/terrence/moz/branch/api_rooting/mozilla/js/src/gc/Marking.cpp:1041
#7  PushMarkStack (thing=0xf782d180, gcmarker=0x937e170) at /home/terrence/moz/branch/api_rooting/mozilla/js/src/gc/Marking.cpp:819
#8  MarkInternal<JSScript> (trc=0x937e170, thingp=0xf7833694) at /home/terrence/moz/branch/api_rooting/mozilla/js/src/gc/Marking.cpp:193
#9  0x0817736e in JSFunction::trace (this=(JSFunction * const) 0xf7833680 [object Function "diff"], trc=0x937e170) at /home/terrence/moz/branch/api_rooting/mozilla/js/src/jsfun.cpp:514
#10 0x080ee5d8 in js::GCMarker::processMarkStackTop (this=this@entry=0x937e170, budget=...) at /home/terrence/moz/branch/api_rooting/mozilla/js/src/gc/Marking.cpp:1480
#11 0x080ea434 in js::GCMarker::drainMarkStack (this=0x937e170, budget=...) at /home/terrence/moz/branch/api_rooting/mozilla/js/src/gc/Marking.cpp:1533
#12 0x08179c21 in DrainMarkStack (rt=rt@entry=0x937df40, sliceBudget=..., phase=phase@entry=js::gcstats::PHASE_MARK) at /home/terrence/moz/branch/api_rooting/mozilla/js/src/jsgc.cpp:3869
#13 0x08187233 in IncrementalCollectSlice (rt=rt@entry=0x937df40, budget=0x883c2b0, reason=reason@entry=JS::gcreason::DESTROY_CONTEXT, gckind=gckind@entry=js::GC_NORMAL) at /home/terrence/moz/branch/api_rooting/mozilla/js/src/jsgc.cpp:4385
#14 0x08187fdc in GCCycle (rt=rt@entry=0x937df40, incremental=incremental@entry=0x0, budget=0x0, gckind=gckind@entry=js::GC_NORMAL, reason=reason@entry=JS::gcreason::DESTROY_CONTEXT) at /home/terrence/moz/branch/api_rooting/mozilla/js/src/jsgc.cpp:4555
#15 0x08188534 in Collect (rt=rt@entry=0x937df40, incremental=incremental@entry=0x0, budget=0x0, gckind=gckind@entry=js::GC_NORMAL, reason=reason@entry=JS::gcreason::DESTROY_CONTEXT) at /home/terrence/moz/branch/api_rooting/mozilla/js/src/jsgc.cpp:4699
#16 0x081889cd in js::GC (rt=0x937df40, gckind=js::GC_NORMAL, reason=JS::gcreason::DESTROY_CONTEXT) at /home/terrence/moz/branch/api_rooting/mozilla/js/src/jsgc.cpp:4724
#17 0x08148070 in js::DestroyContext (cx=0x9390ce0, mode=js::DCM_FORCE_GC) at /home/terrence/moz/branch/api_rooting/mozilla/js/src/jscntxt.cpp:267
#18 0x080f98bc in JS_DestroyContext (cx=0x9390ce0) at /home/terrence/moz/branch/api_rooting/mozilla/js/src/jsapi.cpp:858
#19 0x0804ef4d in DestroyContext (cx=cx@entry=0x9390ce0, withGC=withGC@entry=0x1) at /home/terrence/moz/branch/api_rooting/mozilla/js/src/shell/js.cpp:5316
#20 0x0805fe4e in main (argc=0x3, argv=0xffffcb84, envp=0xffffcb94) at /home/terrence/moz/branch/api_rooting/mozilla/js/src/shell/js.cpp:5951

So it looks like the |type| field a baseline script's stub of kind SetElem_DenseAdd is getting set to zero.

Comment 5

[Terrence Cole [:terrence]]

Attachment: oom_missing_check_on_getType-wip0.diff

The issue was a missing handler on a failed getType when generating the baseline stub. Unfortunately, this exposes a second issue: we're not always doing a syntax parse of the method with the invalid delete.

Comment 6

[Terrence Cole [:terrence]]

Or rather, my breakpoint was failing to hit in an opt build. Re-doing in a debug build, we have a better picture.

This is indeed doing the SyntaxParse, but because |(b)| is wrapped in parens, we're getting into |Node SyntaxParser::setInParens(Node pn) { return NodeGeneric; }|. Thus when we test handler.isName in TOK_DELETE, we find |delete <GenericNode>|, rather than |delete <NameNode>| and fail to error. |setInParens| has the comment "String literals enclosed by parentheses are ignored during strict mode parsing", which appears to not be quite true when generating errors.

Comment 7

[Terrence Cole [:terrence]]

Attachment: oom_932530-v0.diff

Lets check in the first fix now without the test. I'll file a new bug to track the second issue.

Comment 8

[Kannan Vijayan [:djvj]]

Comment on attachment 825963 [details] [diff] [review]
oom_932530-v0.diff

Review of attachment 825963 [details] [diff] [review]:
-----------------------------------------------------------------

Right.. getType is fallible..

Comment 9

[Terrence Cole [:terrence]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/173d837f2b5f

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/173d837f2b5f

Comment 11

[Christian Holler (:decoder)]

Fwiw, I am still seeing exactly this signature in fuzzing (both the assertion and the crash). Could it be that we have more places where we don't properly check that fallible call?

Comment 12

[Christian Holler (:decoder)]

I digged through the js code and found that most return values are checked immediately. However, these three occurrences in the baseline compiler might need a fix:

http://mxr.mozilla.org/mozilla-central/source/js/src/jit/BaselineIC.cpp#4733
http://mxr.mozilla.org/mozilla-central/source/js/src/jit/BaselineIC.cpp#9330
http://mxr.mozilla.org/mozilla-central/source/js/src/jit/BaselineIC.h#4953

Let me know if that helps :)

Comment 13

[Terrence Cole [:terrence]]

Thanks Christian! I made a note to do exactly this today; now I can move right to patching.

Comment 14

[Terrence Cole [:terrence]]

Attachment: handle_more_gettype_checks-v0.diff

And the others Christian dug up.

Comment 15

[Terrence Cole [:terrence]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/208198d2bbdd

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/208198d2bbdd

Comment 17

[Ioana (away)]

Not reproducible locally with the testcase in comment 0 - tried with the 10/30 Fx28 JS shell on Ubuntu 13.04 32bit.

I am also not seeing this signature in Socorro. Please let me know if there is anything more specific I need to do to reproduce and verify this bug.