1234663 - Crash [@ js::jit::ICStub::markCode] with shared stubs

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 388bdc46ba51 (build with --enable-optimize --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --target=i686-pc-linux-gnu --disable-tests --enable-debug, run with --fuzzing-safe --no-threads --ion-eager --gc-zeal=14 --ion-shared-stubs=on):

try {
  mathy2 = function() this.__defineSetter__("", function() {});
} catch(e) {}
try {
  mathy3 = function() (mathy2(), evalcx) | 0;
  testMathyFunction(mathy3);
} catch(e) {};
function testMathyFunction(f) {
  for (;;) f();
}

Backtrace:

Program received signal SIGSEGV, Segmentation fault.
js::jit::ICStub::markCode (this=this@entry=0xf7a217d8, trc=trc@entry=0xffffb0a8, name=name@entry=0x8b20c5b "shared-stub-jitcode") at js/src/jit/SharedIC.cpp:148
#0  js::jit::ICStub::markCode (this=this@entry=0xf7a217d8, trc=trc@entry=0xffffb0a8, name=name@entry=0x8b20c5b "shared-stub-jitcode") at js/src/jit/SharedIC.cpp:148
#1  0x08422330 in js::jit::ICStub::trace (this=this@entry=0xf7a217d8, trc=trc@entry=0xffffb0a8) at js/src/jit/SharedIC.cpp:163
#2  0x08424004 in js::jit::ICEntry::trace (this=0xf7a55440, trc=trc@entry=0xffffb0a8) at js/src/jit/SharedIC.cpp:100
#3  0x082db479 in js::jit::IonScript::trace (this=0xf7a55000, trc=0xffffb0a8) at js/src/jit/Ion.cpp:1067
#4  0x082dbcea in js::jit::IonScript::Trace (trc=<optimized out>, script=<optimized out>) at js/src/jit/Ion.cpp:1259
#5  0x0834baea in MarkIonJSFrame (frame=..., trc=<optimized out>) at js/src/jit/JitFrames.cpp:1113
#6  MarkJitActivation (activations=..., trc=<optimized out>) at js/src/jit/JitFrames.cpp:1554
#7  js::jit::MarkJitActivations (rt=0xf7a39000, trc=trc@entry=0xffffb0a8) at js/src/jit/JitFrames.cpp:1586
#8  0x0885f027 in js::gc::GCRuntime::markRuntime (this=this@entry=0xf7a39214, trc=trc@entry=0xffffb0a8, traceOrMark=traceOrMark@entry=js::gc::GCRuntime::MarkRuntime) at js/src/gc/RootMarking.cpp:315
#9  0x08568779 in js::gc::GCRuntime::updatePointersToRelocatedCells (this=this@entry=0xf7a39214, zone=zone@entry=0xf7a54800) at js/src/jsgc.cpp:2697
#10 0x085897db in js::gc::GCRuntime::compactPhase (this=this@entry=0xf7a39214, reason=reason@entry=JS::gcreason::DEBUG_GC, sliceBudget=...) at js/src/jsgc.cpp:5652
#11 0x08589b7c in js::gc::GCRuntime::incrementalCollectSlice (this=this@entry=0xf7a39214, budget=..., reason=reason@entry=JS::gcreason::DEBUG_GC) at js/src/jsgc.cpp:6107
#12 0x0858a9e1 in js::gc::GCRuntime::gcCycle (this=this@entry=0xf7a39214, nonincrementalByAPI=nonincrementalByAPI@entry=true, budget=..., reason=reason@entry=JS::gcreason::DEBUG_GC) at js/src/jsgc.cpp:6282
#13 0x0858af39 in js::gc::GCRuntime::collect (this=this@entry=0xf7a39214, nonincrementalByAPI=nonincrementalByAPI@entry=true, budget=..., reason=reason@entry=JS::gcreason::DEBUG_GC) at js/src/jsgc.cpp:6388
#14 0x0858b1a2 in js::gc::GCRuntime::gc (this=0xf7a39214, gckind=GC_SHRINK, reason=JS::gcreason::DEBUG_GC) at js/src/jsgc.cpp:6446
#15 0x0858c9a8 in js::gc::GCRuntime::runDebugGC (this=this@entry=0xf7a39214) at js/src/jsgc.cpp:6929
#16 0x0883da4b in js::gc::GCRuntime::gcIfNeededPerAllocation (this=0xf7a39214, cx=cx@entry=0xf7a73020) at js/src/gc/Allocator.cpp:28
#17 0x088469d6 in js::gc::GCRuntime::checkAllocatorState<(js::AllowGC)1> (this=0xf7a39214, cx=0xf7a73020, kind=js::gc::STRING) at js/src/gc/Allocator.cpp:55
#18 0x0884811c in js::Allocate<JSString, (js::AllowGC)1> (cx=cx@entry=0xf7a73020) at js/src/gc/Allocator.cpp:211
#19 0x0877c889 in new_<(js::AllowGC)1, unsigned char> (length=39, chars=0xf7ae7100 "function evalcx() {\n    [native code]\n}", cx=0xf7a73020) at js/src/vm/String-inl.h:223
#20 js::NewStringDontDeflate<(js::AllowGC)1, unsigned char> (cx=cx@entry=0xf7a73020, chars=0xf7ae7100 "function evalcx() {\n    [native code]\n}", length=length@entry=39) at js/src/vm/String.cpp:1079
#21 0x0878a902 in FinishStringFlat<unsigned char, mozilla::Vector<unsigned char, 64u, js::TempAllocPolicy> > (cb=..., sb=..., cx=0xf7a73020) at js/src/vm/StringBuffer.cpp:87
#22 js::StringBuffer::finishString (this=this@entry=0xffffb5e8) at js/src/vm/StringBuffer.cpp:128
#23 0x0857a15a in js::FunctionToString (cx=cx@entry=0xf7a73020, fun=fun@entry=..., lambdaParen=true) at js/src/jsfun.cpp:1104
#24 0x0857a8a4 in fun_toStringHelper (cx=0xf7a73020, obj=..., indent=0) at js/src/jsfun.cpp:1122
#25 0x0857aa01 in js::fun_toString (cx=0xf7a73020, argc=0, vp=0xffffb870) at js/src/jsfun.cpp:1140
#26 0x086fe1aa in js::CallJSNative (cx=0xf7a73020, native=0x857a910 <js::fun_toString(JSContext*, unsigned int, JS::Value*)>, args=...) at js/src/jscntxtinlines.h:235
#27 0x086f6cb1 in js::Invoke (cx=0xf7a73020, args=..., construct=construct@entry=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:460
#28 0x086f7972 in js::Invoke (cx=cx@entry=0xf7a73020, thisv=..., fval=..., argc=argc@entry=0, argv=argv@entry=0x0, rval=rval@entry=...) at js/src/vm/Interpreter.cpp:512
#29 0x085aaac8 in MaybeCallMethod (cx=cx@entry=0xf7a73020, obj=obj@entry=..., id=id@entry=..., vp=vp@entry=...) at js/src/jsobj.cpp:3004
#30 0x085ac8ad in JS::OrdinaryToPrimitive (cx=cx@entry=0xf7a73020, obj=obj@entry=..., hint=hint@entry=JSTYPE_NUMBER, vp=vp@entry=...) at js/src/jsobj.cpp:3087
#31 0x085b05b5 in js::ToPrimitiveSlow (cx=cx@entry=0xf7a73020, preferredType=preferredType@entry=JSTYPE_NUMBER, vp=vp@entry=...) at js/src/jsobj.cpp:3134
#32 0x085bb4b9 in ToPrimitive (vp=..., preferredType=JSTYPE_NUMBER, cx=0xf7a73020) at js/src/jsobj.h:1031
#33 js::ToNumberSlow (cx=0xf7a73020, v=..., out=0xffffbaa0) at js/src/jsnum.cpp:1540
#34 0x085bb699 in js::ToNumberSlow (cx=cx@entry=0xf7a73020, v=..., out=out@entry=0xffffbaa0) at js/src/jsnum.cpp:1554
#35 0x085bbc87 in js::ToInt32Slow (cx=cx@entry=0xf7a73020, v=v@entry=..., out=out@entry=0xffffbae0) at js/src/jsnum.cpp:1641
#36 0x0828beff in ToInt32 (out=0xffffbae0, v=..., cx=0xf7a73020) at ../../dist/include/js/Conversions.h:163
#37 js::BitOr (cx=0xf7a73020, lhs=..., rhs=..., out=0xffffbb4c) at js/src/vm/Interpreter-inl.h:716
#38 0xf7fcb670 in ?? ()
Backtrace stopped: previous frame inner to this frame (corrupt stack?)
eax	0xe5e5e5e5	-437918235
ebx	0x9803430	159396912
ecx	0xf7a55440	-140159936
edx	0xf7a55440	-140159936
esi	0xffffb0a8	-20312
edi	0xffffb0a8	-20312
ebp	0xffffada8	4294946216
esp	0xffffad80	4294946176
eip	0x8422247 <js::jit::ICStub::markCode(JSTracer*, char const*)+23>
=> 0x8422247 <js::jit::ICStub::markCode(JSTracer*, char const*)+23>:	mov    -0x4(%eax),%edx
   0x842224a <js::jit::ICStub::markCode(JSTracer*, char const*)+26>:	cmp    (%edx),%eax


Both jsfunfuzz and LangFuzz hit this bug for some time but I could never isolate a proper testcase. Now I was able to get this nice test out of a jsfunfuzz report (even though it was flagged as not reproducible). Please look into this quickly while it's reproducible. Since the crash address pattern looks weird (0xe5e5), marking this one s-s.

Comment 1

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

Hannes works on --ion-shared-stubs=on bugs for now, setting needinfo? from him as a start.

Comment 2

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

=== Treeherder Build Bisection Results by autoBisect ===

The "good" changeset has the timestamp "20151117080511" and the hash "fe34d01fb2ecb2dd4cda82e788cf7b541d5cbdb4".
The "bad" changeset has the timestamp "20151117094304" and the hash "c6139e8bad12b756a178dd7eb005c82cf247bd43".

Likely regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=fe34d01fb2ecb2dd4cda82e788cf7b541d5cbdb4&tochange=c6139e8bad12b756a178dd7eb005c82cf247bd43

Guessing bug 1214508 is the culprit here.

Comment 3

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

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/54b59d69c085
user:        Hannes Verschore
date:        Tue Nov 17 17:57:49 2015 +0100
summary:     Bug 1214508: SharedStubs - Part 3: Enable the getprop stubs in ionmonkey, r=jandem

Comment 4

[Fuzzing Team]

JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.

Comment 5

[Hannes Verschore [:h4writer]]

Upon investigating only shared stubs for ionmonkey is affected (which is not enabled by default). Ergo opening this.

Comment 6

[Hannes Verschore [:h4writer]]

Attachment: Use the correct fallbackStubSpace

We defaulted to use the fallbackspace of the baselinescript for scripted calls. Instead of using the fallbackStubSpace of the ion script when needed. As a result I had to make a destinction between the outermost script (where the ion script lives) and the innermost script (where the code is currently executing). In baseline this is the same, since we don't have inlined functions there. Ion does inline things...

Comment 8

[Jan de Mooij [:jandem]]

Comment on attachment 8704046 [details] [diff] [review]
Use the correct fallbackStubSpace

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

Storing the inner script in IonICEntry makes sense. Storing the outer script feels wasteful because all entries have the same outerScript, so there must be some way to avoid it. Maybe instead of passing a nullable BaselineFrame*, we could pass either 'HandleScript outerScript' or some kind of tagged pointer that's either a BaselineFrame* or something different (pointer to CalleeToken on the stack?) for Ion?

Not very urgent, but maybe we can think about it or file a follow-up bug.

::: js/src/jit/SharedIC.cpp
@@ +919,5 @@
>  }
>  
> +template<typename T>
> +static JSScript*
> +SharedStubOuterScript(BaselineFrame* frame, T* stub)

Nit: s/frame/maybeFrame, to make it clear it can be nullptr?

::: js/src/jit/SharedIC.h
@@ +338,5 @@
>  
>  class IonICEntry : public ICEntry
>  {
>    JSScript* script_;
> +  JSScript* outerScript_;

Nit: indentation is wrong

@@ +1063,5 @@
> +        if (ICStub::CanMakeCalls(kind)) {
> +            if (engine == ICStubCompiler::Engine::Baseline)
> +                return outerScript->baselineScript()->fallbackStubSpace();
> +            else
> +                return outerScript->ionScript()->fallbackStubSpace();

Nit: no else after return

Comment 9

[Hannes Verschore [:h4writer]]

Attachment: Use jitstack

This is based on the previous patch. Upon r+ I'll commit them as one patch.

- Cleans up some code, by having SharedStubInfo class
- has "outerScript" that lazily retrieves outerScript using iterator. This is only used/needed when adding a new stub. I'm not too worried it takes a lot of time. And it is cleaner than adding "JSScript*" to every ICStubCompiler constructor.
- renamed pushFramePtr to pushStubPayload.

Comment 10

[Hannes Verschore [:h4writer]]

Comment on attachment 8709098 [details] [diff] [review]
Use jitstack

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

::: js/src/jit/JitOptions.cpp
@@ +110,5 @@
>      // Toggle whether eager scalar replacement is globally disabled.
>      SET_DEFAULT(disableScalarReplacement, false);
>  
>      // Toggles whether shared stubs are used in Ionmonkey.
> +    SET_DEFAULT(disableSharedStubs, false);

Undo this code

@@ +127,5 @@
>      // Whether functions are compiled immediately.
>      SET_DEFAULT(eagerCompilation, false);
>  
>      // Whether IonBuilder should prefer IC generation above specialized MIR.
> +    SET_DEFAULT(forceInlineCaches, true);

Undo this code

Comment 11

[Hannes Verschore [:h4writer]]

Comment on attachment 8709098 [details] [diff] [review]
Use jitstack

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

::: js/src/jit/SharedIC.cpp
@@ +1636,5 @@
>                    HandleValue rhs, MutableHandleValue ret)
>  {
> +    SharedStubInfo info(cx, payload, stub_->icEntry());
> +    ICStubCompiler::Engine engine = info.engine();
> +    HandleScript script = info.innerScript();

script is not used. Remove in order to have no warning.

Comment 12

[Jan de Mooij [:jandem]]

Comment on attachment 8709098 [details] [diff] [review]
Use jitstack

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

Nice idea!

::: js/src/jit/SharedIC.cpp
@@ +909,2 @@
>  {
> +    if (((intptr_t)payload & 1) == 0 && payload != nullptr) {

This can be |if (payload) {| I think.

@@ +913,5 @@
> +        innerScript_ = maybeFrame_->script();
> +    } else {
> +        IonICEntry* entry = (IonICEntry*) icEntry;
> +        innerScript_ = entry->script();
> +        // outerScript_ is initialized lazyly.

Nit: lazily

::: js/src/jit/SharedIC.h
@@ +1075,5 @@
> +
> +  public:
> +    SharedStubInfo(JSContext* cx, void* payload, ICEntry* entry);
> +
> +    ICStubCompiler::Engine engine() {

Nit: I think all these methods (except outerScript) can be 'const'

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/650d17df486e

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/650d17df486e