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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

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

Attachment: debug stack

var f = bindToAsyncStack(
  function () {
    callFunctionWithAsyncStack(
      function () {
        Error.captureStackTrace(
          function () {},
          function () {},
        );
      },
      saveStack(),
      "x",
    );
  },
  { stack: saveStack() },
);
(function () {
  f();
})();

(gdb) bt
#0  0x00005555576c807c in MOZ_CrashSequence (aAddress=0x0, aLine=1499) at /home/msf1/shell-cache/js-dbg-64-linux-x86_64-7f68e3a765cf/objdir-js/dist/include/mozilla/Assertions.h:248
#1  js::SavedStacks::insertFrames (this=0x7ffff5e91270, cx=0x7ffff693a200, frame=..., capture=..., startAtObj=startAtObj@entry=...) at /home/msf1/trees/mozilla-central/js/src/vm/SavedStacks.cpp:1498
#2  0x00005555576c69ee in js::SavedStacks::saveCurrentStack (this=<optimized out>, cx=<optimized out>, frame=..., capture=..., startAt=...) at /home/msf1/trees/mozilla-central/js/src/vm/SavedStacks.cpp:1339
#3  0x00005555579a55d5 in JS::CaptureCurrentStack (cx=cx@entry=0x7ffff693a200, stackp=stackp@entry=..., capture=..., startAt=startAt@entry=...) at /home/msf1/trees/mozilla-central/js/src/jsapi.cpp:4952
#4  0x00005555574c3d08 in exn_captureStackTrace (cx=cx@entry=0x7ffff693a200, argc=<optimized out>, vp=<optimized out>) at /home/msf1/trees/mozilla-central/js/src/vm/ErrorObject.cpp:1081
#5  0x0000555557323b75 in CallJSNative (cx=cx@entry=0x7ffff693a200, native=0x5555574c3ab0 <exn_captureStackTrace(JSContext*, unsigned int, JS::Value*)>, reason=<optimized out>, args=...) at /home/msf1/trees/mozilla-central/js/src/vm/Interpreter.cpp:494
/snip

When run with --fuzzing-safe --no-threads --no-baseline --no-ion:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/5fdd7a5d986d
user:        Matthew Gaudet
date:        Mon Mar 03 15:48:35 2025 +0000
summary:     Bug 1950508 - Ship Error.captureStackTrace r=dminor,mccr8

When run with --fuzzing-safe --no-threads --no-baseline --no-ion --setpref=experimental.error_capture_stack_trace=true:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/c27c123c7051
user:        Matthew Gaudet
date:        Thu Jan 16 15:31:28 2025 +0000
summary:     Bug 1886820 - Add experimental support for Error.captureStackTrace r=jandem

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 7f68e3a765cf.

Matt, is bug 1950508 a likely regressor?

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1950508

Comment 2

[Matthew Gaudet (he/him) [:mgaudet]]

(This is a diagnostic assert. It's unlikely to be a sec bug, but I'll get back to this Monday as I likely won't have time today and I'm off tomorrow)

Comment 3

[Matthew Gaudet (he/him) [:mgaudet]]

This is yet another cached frame assertion issue. Not security sensitive.

This one is awkward as all get-out unfortunately. I'm not entirely sure of what the fix is just yet.

So here's a slightly reduced test case

let s = undefined;
var f = bindToAsyncStack(
    function () {
       // (a) 
        function b () {
          Error.captureStackTrace( // (4)
            function () {},
            function () {},
          );
        };
        s = saveStack(); // (3) 
        b();
    },
    { stack: saveStack() }, // (1) 
  );

  function g() { 
    // (2)
    f();
  }
g();

So what I am seeing is the following:

(1) First we capture the stack. This inserts the top-level frame into the saved frame cache. This is an AllFrames capture, and thus will populate the cache.
(2) Then we call the async function with it's async activation set, but with a new parent. This frame has not been captured yet.
(3) We call saveStack again. This is also an AllFrames capture, however I don't see it capuring the call to g: The trace shows ups adopting the async stack, and becasue the unreached eval targets is empty, we terminate the stack capture here, before we see g
(4) We call Error.captureStackTrace. This walks the stack using the MaxFrames walker: It will -not- fill in the cache. However, because we've tried to avoid adopting aync stacks where we've not seen the callee frame, we skip over the async stack and then go forth and walk down to g which has no cached frame, and hit the assert.

So, in this case the assert is not really valuable. We can provide a bandaid fix.

Comment 4

[Matthew Gaudet (he/him) [:mgaudet]]

So I think fixing -part- of Bug 1961684 is probably the better way forward here: Rather than trying to special case what happens when you traverse the regular stack... just stop traversing the regular stack if you haven't found the caller when you see an async parent.

Comment 5

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Bug 1961019 - Don't continue to traverse regular stack when async stack exists for Error.captureStackTrace r?jandem

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e8d0fd35c0e2
Don't continue to traverse regular stack when async stack exists for Error.captureStackTrace r=jandem

Comment 7

[Serban Stanca [:SerbanS]]

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