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

RESOLVED FIXED in mozilla28

Status

()

Core
JavaScript Engine
--
critical
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: decoder, Assigned: terrence)

Tracking

(Blocks: 2 bugs, {assertion, crash, testcase})

Trunk
mozilla28
x86
Linux
assertion, crash, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(crash signature)

Attachments

(4 attachments)

(Reporter)

Description

4 years ago
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);
(Reporter)

Comment 1

4 years ago
Created attachment 824313 [details]
[crash-signature] Machine-readable crash signature
(Reporter)

Comment 2

4 years ago
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?
Crash Signature: [@ tenuredZone]
Flags: needinfo?(terrence)
Keywords: crash
Summary: Assertion failure: thing, at gc/Marking.cpp:126 with OOM → Assertion failure: thing, at gc/Marking.cpp:126 or Crash [@ tenuredZone] with OOM
(Assignee)

Updated

4 years ago
Blocks: 912928
Flags: needinfo?(terrence)
(Assignee)

Comment 3

4 years ago
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.
(Assignee)

Comment 4

4 years ago
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.
(Assignee)

Comment 5

4 years ago
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.
(Assignee)

Comment 6

4 years ago
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.
(Assignee)

Comment 7

4 years ago
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.
Assignee: general → terrence
Status: NEW → ASSIGNED
Attachment #825963 - Flags: review?(kvijayan)
(Assignee)

Updated

4 years ago
See Also: → bug 933809
Comment on attachment 825963 [details] [diff] [review]
oom_932530-v0.diff

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

Right.. getType is fallible..
Attachment #825963 - Flags: review?(kvijayan) → review+
(Assignee)

Comment 9

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/173d837f2b5f
https://hg.mozilla.org/mozilla-central/rev/173d837f2b5f
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
(Reporter)

Comment 11

4 years ago
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?
Flags: needinfo?(terrence)
(Reporter)

Comment 12

4 years ago
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 :)
(Assignee)

Comment 13

4 years ago
Thanks Christian! I made a note to do exactly this today; now I can move right to patching.
Flags: needinfo?(terrence)
(Assignee)

Comment 14

4 years ago
Created attachment 826888 [details] [diff] [review]
handle_more_gettype_checks-v0.diff

And the others Christian dug up.
Attachment #826888 - Flags: review?(kvijayan)
(Assignee)

Updated

4 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Updated

4 years ago
Attachment #826888 - Flags: review?(kvijayan) → review+
(Assignee)

Comment 15

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/208198d2bbdd
https://hg.mozilla.org/mozilla-central/rev/208198d2bbdd
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED

Updated

3 years ago
Keywords: verifyme

Comment 17

3 years ago
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.
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.