1505387 - Assertion failure: target.hasCachedSavedFrame(), at js/src/vm/SavedStacks.cpp:1690 with Debugger

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision d2963b5a2897 (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --disable-profiling --enable-debug --enable-optimize, run with --fuzzing-safe --cpu-count=2 --ion-offthread-compile=off --ion-eager):

var evalInFrame = (function (global) {
  var dbgGlobal = newGlobal();
  var dbg = new dbgGlobal.Debugger();
  return function evalInFrame(upCount, code) {
    dbg.addDebuggee(global);
    var frame = dbg.getNewestFrame().older;
      frame = frame.older;
    var completion = frame.eval(code);
  };
})(this);
function isObject(obj) {}
function assertStackIs(x) {
    return new Promise(resolve => {});
}
f = isObject;
var obj2 = {};
obj2['b' + {}] = 0;
for (var k in obj2)
  (function g() { evalInFrame(1, "assertStackIs(['eval-code', f, 'bound(f)', 'global-code'])", true); })();


Backtrace:

received signal SIGSEGV, Segmentation fault.
0x0000555555b23006 in js::SavedStacks::checkForEvalInFramePrev (this=this@entry=0x7ffff5f821e8, cx=0x7ffff5f18000, lookup=lookup@entry=...) at js/src/vm/SavedStacks.cpp:1690
#0  0x0000555555b23006 in js::SavedStacks::checkForEvalInFramePrev (this=this@entry=0x7ffff5f821e8, cx=0x7ffff5f18000, lookup=lookup@entry=...) at js/src/vm/SavedStacks.cpp:1690
#1  0x0000555555b376a4 in js::SavedStacks::insertFrames(JSContext*, JS::MutableHandle<js::SavedFrame*>, mozilla::Variant<JS::AllFrames, JS::MaxFrames, JS::FirstSubsumedFrame>&&) (this=this@entry=0x7ffff5f821e8, cx=<optimized out>, cx@entry=0x7ffff5f18000, frame=..., capture=capture@entry=<unknown type>) at js/src/vm/SavedStacks.cpp:1572
#2  0x0000555555b381ee in js::SavedStacks::saveCurrentStack(JSContext*, JS::MutableHandle<js::SavedFrame*>, mozilla::Variant<JS::AllFrames, JS::MaxFrames, JS::FirstSubsumedFrame>&&) (this=0x7ffff5f821e8, cx=cx@entry=0x7ffff5f18000, frame=..., capture=capture@entry=<unknown type>) at js/src/vm/SavedStacks.cpp:1289
#3  0x0000555555d23645 in JS::CaptureCurrentStack(JSContext*, JS::MutableHandle<JSObject*>, mozilla::Variant<JS::AllFrames, JS::MaxFrames, JS::FirstSubsumedFrame>&&) (cx=cx@entry=0x7ffff5f18000, stackp=stackp@entry=..., capture=capture@entry=<unknown type>) at js/src/jsapi.cpp:6781
#4  0x0000555555942740 in PromiseDebugInfo::create (cx=cx@entry=0x7ffff5f18000, promise=promise@entry=..., maybeNow=...) at js/src/builtin/Promise.cpp:295
#5  0x0000555555922f2a in CreatePromiseObjectInternal (cx=0x7ffff5f18000, proto=..., proto@entry=..., protoIsWrapped=protoIsWrapped@entry=false, informDebugger=informDebugger@entry=false) at js/src/builtin/Promise.cpp:1982
#6  0x00005555559258bb in js::PromiseObject::create (cx=<optimized out>, executor=executor@entry=..., proto=..., proto@entry=..., needsWrapping=<optimized out>) at js/src/builtin/Promise.cpp:2118
#7  0x0000555555926082 in PromiseConstructor (cx=<optimized out>, argc=<optimized out>, vp=<optimized out>) at js/src/builtin/Promise.cpp:2082
#8  0x00005555558ac175 in CallJSNative (cx=0x7ffff5f18000, native=native@entry=0x555555925ec0 <PromiseConstructor(JSContext*, unsigned int, JS::Value*)>, args=...) at js/src/vm/Interpreter.cpp:468
#9  0x00005555558ac369 in CallJSNativeConstructor (cx=<optimized out>, native=0x555555925ec0 <PromiseConstructor(JSContext*, unsigned int, JS::Value*)>, args=...) at js/src/vm/Interpreter.cpp:485
#10 0x00005555558a06d4 in InternalConstruct (cx=<optimized out>, args=...) at js/src/vm/Interpreter.cpp:661
#11 0x00005555558a084d in js::ConstructFromStack (cx=<optimized out>, args=...) at js/src/vm/Interpreter.cpp:707
#12 0x0000555555f8f72e in js::jit::DoCallFallback (cx=<optimized out>, frame=0x7fffffff8cc8, stub_=<optimized out>, argc=<optimized out>, vp=0x7fffffff8c68, res=...) at js/src/jit/BaselineIC.cpp:3666
#13 0x00002e2c3c54ff03 in ?? ()
[...]
#37 0x0000000000000000 in ?? ()
rax	0x0	0
rbx	0x7fffffff78d0	140737488320720
rcx	0x7ffff6c1c2dd	140737333281501
rdx	0x0	0
rsi	0x7ffff6eeb770	140737336227696
rdi	0x7ffff6eea540	140737336223040
rbp	0x7fffffff7670	140737488320112
rsp	0x7fffffff75e0	140737488319968
r8	0x7ffff6eeb770	140737336227696
r9	0x7ffff7fe6cc0	140737354034368
r10	0x58	88
r11	0x7ffff6b927a0	140737332717472
r12	0x0	0
r13	0x7ffff5f18000	140737319632896
r14	0x7fffffff7620	140737488320032
r15	0x7fffffff78d0	140737488320720
rip	0x555555b23006 <js::SavedStacks::checkForEvalInFramePrev(JSContext*, js::SavedFrame::HandleLookup)+822>
=> 0x555555b23006 <js::SavedStacks::checkForEvalInFramePrev(JSContext*, js::SavedFrame::HandleLookup)+822>:	movl   $0x0,0x0
   0x555555b23011 <js::SavedStacks::checkForEvalInFramePrev(JSContext*, js::SavedFrame::HandleLookup)+833>:	ud2

Comment 1

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

autobisectjs shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/1a7cb8857de4
user:        Jim Blandy
date:        Wed Mar 14 12:54:04 2018 -0700
summary:     Bug 1444604: Part 7: Reconcile LiveSavedFrameCache with evalInFramePrev by tweaking SavedFrame parent links. r=jorendorff

Jim, is bug 1444604 a likely regressor?

Comment 2

[Jim Blandy :jimb]

I can reproduce this. Taking.

Comment 3

[Jim Blandy :jimb]

Somewhat reduced:

var evalInFrame = (function (global) {
  var dbgGlobal = newGlobal();
  var dbg = new dbgGlobal.Debugger();
  return function evalInFrame(code) {
    dbg.addDebuggee(global);
    var frame = dbg.getNewestFrame().older;
      frame = frame.older;
    var completion = frame.eval(code);
  };
})(this);

while (true) {
  (function g() { evalInFrame("saveStack()"); })();
}

Comment 4

[Jim Blandy :jimb]

We're calling saveStack from a Debugger 'eval' frame evaluating in the test script's top-level frame. Thus, the Debugger 'eval' frame's actual parent is the call to evalInFrame, but its evalInFramePrev link refers to the top-level frame. This is fine.

However, there's something interesting. The stack as seen by SavedStacks::insertFrame's FrameIter consists of jit::CommonFrameLayout frames, which I think are Ion frames (the --ion-eager flag is required, to reproduce), yet the Debugger 'eval' frame's 'evalInFramePrev' link refers to a jit::RematerializedFrame. This upsets SavedStacks::checkForEvalInFramePrev, which expects that, since it is walking frames from oldest to youngest, bringing the LiveSavedFrameCache up to date as it goes, 'evalInFramePrev' must refer to something in the cache.

A RematerializedFrame is built from an Ion frame, to give Debugger something it can work with. An Ion frame may represent several inlined JavaScript frames, and omit variables that one would expect to find, based on the source code. Unpacking an Ion frame to produce one or more RematerializedFrames uses the same information an Ion bailout would use to build a separate RematerializedFrames for each inlined call, with all variables present and usable. SpiderMonkey puts off unpacking Ion frames into RematerializedFrames until Debugger asks for something the Ion frame can't easily support - like an eval.

Since Debugger can assign to variables in a RematerializedFrame, they should be considered the canonical source of truth once they are created. It seems that FrameIter should not be producing the old jit::CommonFrameLayout frames once the RematerializedFrames exist.

Comment 5

[Jim Blandy :jimb]

Well, that's not quite right - if one needs variable values, then yes, the RematerializedFrames must be used. But for simply producing stack traces, anything that produces inline frames (as FrameIter does) should be fine.

Comment 6

[Jim Blandy :jimb]

I'm kind of excited about this, because it seems like we should trigger this assertion any time we try to save a stack in a Debugger.Frame eval. The following asserts, too:

    var evalInFrame = (function (global) {
      var dbgGlobal = newGlobal();
      var dbg = new dbgGlobal.Debugger(global);
      return function evalInFrame(code) {
        let frame = dbg.getNewestFrame().older.older;
        print(frame.script.callee);
        frame.eval(code);
      };
    })(this);

    while (true) {
      (function g() { evalInFrame("new Error"); })();
    }

Comment 7

[Jim Blandy :jimb]

The symptoms of hitting this bug in production (without the debug-only asserts) would simply be that a captured stack (for an error or promise) would end at the eval frame - it wouldn't have the frame in whose environment we're carrying out the evaluation as a parent. So it doesn't seem terribly consequential.

Comment 8

[Fuzzing Team]

JSBugMon: The testcase found in this bug no longer reproduces (tried revision 3dc7d345da52).

Comment 9

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

var evalInFrame = (function (global) {
  var dbgGlobal = newGlobal();
  var dbg = new dbgGlobal.Debugger();
  return function evalInFrame(code) {
    dbg.addDebuggee(global);
    var frame = dbg.getNewestFrame().older;
      frame = frame.older;
    var completion = frame.eval(code);
  };
})(this);

while (true) {
  (function g() { evalInFrame("saveStack()"); })();
}

asserts js shell compiled with --enable-debug on m-c rev 8ec327de0ba7 using --fuzzing-safe --no-threads --ion-eager --more-compartments at Assertion failure: target.hasCachedSavedFrame(), at js/src/vm/SavedStacks.cpp:1550

Comment 10

[Fuzzing Team]

JSBugMon: Cannot process bug: Error: Failed to isolate test from comment

Comment 11

[Jim Blandy :jimb]

I haven't been working on this, since it's P3 (per comment 7).

Comment 12

[Intermittent Failures Robot]

1 failures in 4850 pushes (0.0 failures/push) were associated with this bug in the last 7 days.

Repository breakdown:

• autoland: 1

Platform breakdown:

• windows10-64: 1

For more details, see:
https://treeherder.mozilla.org/intermittent-failures.html#/bugdetails?bug=1505387&startday=2019-11-04&endday=2019-11-10&tree=all

Comment 13

[Intermittent Failures Robot]

1 failures in 5444 pushes (0.0 failures/push) were associated with this bug in the last 7 days.

Repository breakdown:

• autoland: 1

Platform breakdown:

• macosx1014-64: 1

For more details, see:
https://treeherder.mozilla.org/intermittent-failures.html#/bugdetails?bug=1505387&startday=2019-11-18&endday=2019-11-24&tree=all

Comment 14

[Jan de Mooij [:jandem]]

I'm hitting this with WarpBuilder enabled on debug/bug-1444604-reduced.js.

The fix for this isn't trivial. I think I'll add --no-warp to that test but it would be great if we could fix this bug.

Comment 15

[Logan Smyth [:loganfsmyth]]

This reproduction can be trimmed down to

var dbgGlobal = newGlobal({ newCompartment: true });
var dbg = new dbgGlobal.Debugger(globalThis);

for (var i = 0; i < 1; i++) {
  (() => {
    dbg.getNewestFrame().older.eval("saveStack()");
  })();
}

run with --fuzzing-safe --no-threads --ion-eager.

I was looking into a related bug and decided to check out this one too. I have a potential fix but I'll need review before I'm particularly confident that it's the right fix.

Comment 16

[Logan Smyth [:loganfsmyth]]

Attachment: Bug 1505387 - Consistently use a FrameIter to produce consistent cache pointers. r=jorendorff!

This bug is caused because creating a LiveSavedFrameCache::FramePtr from a
FrameIter special-cases RematerializedFrames that are also PhysicalJitFrames.
This creates a FramePtr for a CommonFrameLayout, which is reasonable, but it
conflicts with the fact that ExecuteKernel is only passed an AbstractFramePtr
which means that when we go to compare the two FramePtr values, one is a
RematerializedFrame and one is a CommonFrameLayout even though they both
refer to the same frame.

This patch uses the FrameIter construction method for all FramePtr instances
so we avoid this two-constructor approach and can be more easily sure that
all code paths can properly construct the same FramePtr.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/07f09d4d22f7
Consistently use a FrameIter to produce consistent cache pointers. r=jorendorff

Comment 18

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/07f09d4d22f7