Last Comment Bug 932530 - Assertion failure: thing, at gc/Marking.cpp:126 or Crash [@ tenuredZone] with OOM
: Assertion failure: thing, at gc/Marking.cpp:126 or Crash [@ tenuredZone] with...
Status: RESOLVED FIXED
: assertion, crash, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86 Linux
: -- critical (vote)
: mozilla28
Assigned To: Terrence Cole [:terrence]
: general
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks: langfuzz 912928
  Show dependency treegraph
 
Reported: 2013-10-29 15:35 PDT by Christian Holler (:decoder)
Modified: 2014-02-05 02:35 PST (History)
7 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
[crash-signature] Machine-readable crash signature (793 bytes, text/plain)
2013-10-29 15:38 PDT, Christian Holler (:decoder)
no flags Details
oom_missing_check_on_getType-wip0.diff (1.49 KB, patch)
2013-10-31 17:39 PDT, Terrence Cole [:terrence]
no flags Details | Diff | Splinter Review
oom_932530-v0.diff (1.12 KB, patch)
2013-11-01 09:29 PDT, Terrence Cole [:terrence]
kvijayan: review+
Details | Diff | Splinter Review
handle_more_gettype_checks-v0.diff (2.94 KB, patch)
2013-11-04 10:55 PST, Terrence Cole [:terrence]
kvijayan: review+
Details | Diff | Splinter Review

Description User image Christian Holler (:decoder) 2013-10-29 15:35:41 PDT
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 User image Christian Holler (:decoder) 2013-10-29 15:38:47 PDT
Created attachment 824313 [details]
[crash-signature] Machine-readable crash signature
Comment 2 User image Christian Holler (:decoder) 2013-10-29 15:40:43 PDT
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 User image Terrence Cole [:terrence] 2013-10-31 14:57:02 PDT
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 User image Terrence Cole [:terrence] 2013-10-31 16:14:06 PDT
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 User image Terrence Cole [:terrence] 2013-10-31 17:39:00 PDT
Created attachment 825637 [details] [diff] [review]
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 User image Terrence Cole [:terrence] 2013-11-01 08:51:36 PDT
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 User image Terrence Cole [:terrence] 2013-11-01 09:29:04 PDT
Created attachment 825963 [details] [diff] [review]
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 User image Kannan Vijayan [:djvj] 2013-11-01 11:45:25 PDT
Comment on attachment 825963 [details] [diff] [review]
oom_932530-v0.diff

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

Right.. getType is fallible..
Comment 9 User image Terrence Cole [:terrence] 2013-11-01 14:09:23 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/173d837f2b5f
Comment 10 User image Wes Kocher (:KWierso) 2013-11-01 20:39:21 PDT
https://hg.mozilla.org/mozilla-central/rev/173d837f2b5f
Comment 11 User image Christian Holler (:decoder) 2013-11-04 03:03:06 PST
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 User image Christian Holler (:decoder) 2013-11-04 06:56:33 PST
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 User image Terrence Cole [:terrence] 2013-11-04 10:41:56 PST
Thanks Christian! I made a note to do exactly this today; now I can move right to patching.
Comment 14 User image Terrence Cole [:terrence] 2013-11-04 10:55:21 PST
Created attachment 826888 [details] [diff] [review]
handle_more_gettype_checks-v0.diff

And the others Christian dug up.
Comment 15 User image Terrence Cole [:terrence] 2013-11-05 10:05:14 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/208198d2bbdd
Comment 16 User image Wes Kocher (:KWierso) 2013-11-05 17:56:14 PST
https://hg.mozilla.org/mozilla-central/rev/208198d2bbdd
Comment 17 User image Ioana (away) 2014-02-05 02:35:01 PST
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.

Note You need to log in before you can comment on or make changes to this bug.