1907801 - Assertion failure: framePtr->hasCachedSavedFrame() || hasGoodExcuse, at vm/SavedStacks.cpp:1483

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

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

Attachment: debug shell stack

var x = newGlobal({
  newCompartment: true,
});
var y = Debugger(x);
y.onDebuggerStatement = function () {
  return bindToAsyncStack(
    function () {
      for (let m = enableTrackAllocations(); m++; ) {}
      y.getNewestFrame().eval("saveStack()");
    },
    {
      stack: saveStack(true),
    }
  )();
};
x.eval("debugger");

#0  js::SavedStacks::insertFrames (this=this@entry=0x7ffff6180b68, cx=0x7ffff6236100, frame=..., capture=...) at /home/yksubu/trees/mozilla-central/js/src/vm/SavedStacks.cpp:1482
#1  0x00005555576a35a9 in js::SavedStacks::saveCurrentStack (this=<optimized out>, cx=<optimized out>, frame=..., capture=...) at /home/yksubu/trees/mozilla-central/js/src/vm/SavedStacks.cpp:1326
#2  0x0000555557949328 in JS::CaptureCurrentStack (cx=cx@entry=0x7ffff6236100, stackp=stackp@entry=..., capture=...) at /home/yksubu/trees/mozilla-central/js/src/jsapi.cpp:4924
#3  0x000055555788533a in SaveStack (cx=cx@entry=0x7ffff6236100, argc=<optimized out>, vp=<optimized out>) at /home/yksubu/trees/mozilla-central/js/src/builtin/TestingFunctions.cpp:3158
#4  0x00005555572c9c85 in CallJSNative (cx=cx@entry=0x7ffff6236100, native=0x555557884ff0 <SaveStack(JSContext*, unsigned int, JS::Value*)>, reason=reason@entry=js::CallReason::Call, args=...) at /home/yksubu/trees/mozilla-central/js/src/vm/Interpreter.cpp:491
#5  0x00005555572a1a72 in js::InternalCallOrConstruct (cx=0x7ffff6236100, args=..., construct=construct@entry=js::NO_CONSTRUCT, reason=js::CallReason::Call) at /home/yksubu/trees/mozilla-central/js/src/vm/Interpreter.cpp:585
#6  0x00005555572a27a8 in InternalCall (cx=<optimized out>, args=..., reason=1490134096) at /home/yksubu/trees/mozilla-central/js/src/vm/Interpreter.cpp:652
#7  0x00005555572b1896 in js::CallFromStack (cx=0x7ffff7a1ca60 <_IO_stdfile_2_lock>, args=..., reason=<optimized out>) at /home/yksubu/trees/mozilla-central/js/src/vm/Interpreter.cpp:657
#8  js::Interpret (cx=0x7ffff6236100, state=...) at /home/yksubu/trees/mozilla-central/js/src/vm/Interpreter.cpp:3363
#9  0x00005555572a1309 in MaybeEnterInterpreterTrampoline (cx=0x7ffff7a1ca60 <_IO_stdfile_2_lock>, cx@entry=0x7ffff6236100, state=...) at /home/yksubu/trees/mozilla-central/js/src/vm/Interpreter.cpp:405
#10 0x00005555572a0fbf in js::RunScript (cx=cx@entry=0x7ffff6236100, state=...) at /home/yksubu/trees/mozilla-central/js/src/vm/Interpreter.cpp:463
/snip

Bisection is still happening, seems to go as far back as m-c rev c9d807dab8f1 for now.

Run with --fuzzing-safe --no-threads --no-baseline --no-ion, compile with AR=ar sh ../configure --enable-debug --enable-debug-symbols --with-ccache --enable-nspr-build --enable-ctypes --enable-gczeal --enable-rust-simd --disable-tests, tested on m-c rev 8e1eae15c143.

Setting s-s to be safe.

Comment 1

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

This seems to go way back past m-c rev a5887514ddfb5f4aa9b73be6ff67d15a8d3ed062 (Feb 2022), when it showed up as:

Assertion failure: framePtr->hasCachedSavedFrame() || framePtr->isRematerializedFrame(), at vm/SavedStacks.cpp:1452

bindToAsyncStack seems to not have been defined in the last available downloadable js nightly shell from 2015-10-21.

Thus, setting needinfo? from Jan as a start.

Comment 2

[Iain Ireland [:iain]]

I took a quick look at this. It's not s-s; it's just another case where we might need to expand the definition of hasGoodExcuse (or give up on this assertion entirely).

Here's a slightly cleaned-up testcase:

let g = newGlobal({ newCompartment: true });
let dbg = Debugger(g);
dbg.onDebuggerStatement = function () {
  return bindToAsyncStack(
    function () {
      saveStack();
      dbg.getNewestFrame().eval("saveStack()");
    },
    {
      stack: saveStack(1),
    }
  )();
};
g.eval("debugger");

I suspect we are running into a problem with a realm mismatch clearing the cache as described here. Notably, at the time of the failed assertion, the cache is empty. I haven't worked out why it is only a problem in this case; I think it has to do with binding an async stack that only captured a single frame.

Comment 3

[Iain Ireland [:iain]]

This assertion isn't security sensitive.

Comment 4

[Jan de Mooij [:jandem]]

Clearing since Iain has looked into this.

Comment 5

[Iain Ireland [:iain]]

Looked at this a little bit more. Here's my final version of the testcase:

let g = newGlobal({ newCompartment: true });
let dbg = Debugger(g);

function foo() {
  saveStack();
  dbg.getNewestFrame().eval("saveStack()");
}

let stack = saveStack();
dbg.onDebuggerStatement = bindToAsyncStack(foo, {stack: stack});
g.eval("debugger");

When we reach foo, the physical stack looks something like this:

foo
bindToAsyncStack thunk
g.eval("debugger")

Separately, we've captured a stack that doesn't include the eval statement, and bound it as the async parent of foo.

Inside foo, we save the stack twice. The first time, we call saveStack directly. We visit foo and the captured stack, and add foo to the cache. The second time, we evaluate saveStack using the debugger. This has two effects. First, it means that we're in a different realm. Second, it means that there's an additional debugger-eval frame on top of the stack, with an evalInFramePrev pointer pointing at the g.eval("debugger") frame:

debuggerEval -----------------------
foo (cached)                       |
bindToAsyncStack thunk             |
g.eval("debugger") (not cached) <---

We start by visiting the debugger eval frame, which adds an entry to the unreachedEvalTargets list. Next, we visit foo, see that it's already in the cache, and set seenCached to true. We immediately reach this code, fail to find foo in the cache because of the realm mismatch, and clear the hasCachedSavedFrame flag. However, we don't change seenCached.

This time, because there's still an unreached eval target, we don't stop after visiting the async parent stack, because we need to make sure we visit all eval target frames. When we eventually reach that frame, we see that it's uncached, and we've already seen a cached frame, so we assert.

One way to fix this would be to set a debug flag when we clear hasCachedSavedFrame, and treat that as a good excuse along with the others. The other fix would be to get rid of this assertion entirely, which is what I claimed we should do last time there was a bug here.

Comment 6

[Iain Ireland [:iain]]

Attachment: Bug 1907801: Adjust CachedSavedFrame bit assertion r=mgaudet

In bug 1832936, we relaxed this assertion in cases where we had a pending eval-in-frame pointer to a frame we hadn't seen yet, because that can cause us to keep going after we see an async parent (in a way that we wouldn't if we weren't doing eval-in-frame).

The problem is that we can continue seeing new frames, even after we've reached the eval-in-frame target, and one of those frames could be un-cached. Instead of checking for unreached eval targets, this patch just clears seenCached when we continue past an async parent. This means that a) we don't assert that caching a frame with an async parent means that we must have cached its non-async parent, and b) if we've cached any frames past this point, then we will still enforce our invariants on those frames.

Comment 7

[Iain Ireland [:iain]]

I came up with a fix that I think is clean enough to let this assertion live. For now.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b8bfadd7ed56
Adjust CachedSavedFrame bit assertion r=mgaudet

Comment 9

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/b8bfadd7ed56