1418971 - Assertion failure: rematFrame->script() == frame->script(), at js/src/jit/BaselineBailouts.cpp:1816 or Assertion failure: containsPC(pc), at js/src/jsscript.cpp:4060 with Debugger

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision fc194660762d+ (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-stdcxx-compat --disable-profiling --enable-debug --without-intl-api --enable-optimize --target=i686-pc-linux-gnu --enable-simulator=arm, run with --fuzzing-safe --cpu-count=2):

try {
  Object.defineProperty(this, "fuzzutils", {
    value:{}
  });
} catch(exc) {}
gcPreserveCode()
try {
interruptTest(new Function(""));
oomTest(new Function(`
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;
    for (var i = 0; i < upCount; i++) {
      if (!frame.older)
        break;
      frame = frame.older;
    }
    var completion = frame.eval(code);
    if (completion.return) {
      var v = completion.return;
      if (typeof v === "object")
        v = v.unsafeDereference();
      return v;
    }
    if (completion.throw) {
      var v = completion.throw;
      if (typeof v === "object")
        v = v.unsafeDereference();
      throw v;
    }
    if (completion === null)
      terminate();
  };
})(this);
function f() evalInFrame(1, "print(a)");
function g() evalInFrame(1, "a = 42");
function testGet() (function ( v1, ... get) { f(); })();
function testSet() (function () { g(); })();
try {
  testGet();
} catch (e) {}
try {
  testSet();
} catch (e) {}
`));
Array.prototype.set = function(index, value) {}
} catch(exc) {}


Backtrace:

received signal SIGSEGV, Segmentation fault.
0x08ab1b7f in CopyFromRematerializedFrame (frame=0xf5fffdc8, inlineDepth=2, fp=0xf5fffeb0 "\324L\232AB\020", act=0xffffbbe0, cx=0xf6e1d800) at js/src/jit/BaselineBailouts.cpp:1816
#0  0x08ab1b7f in CopyFromRematerializedFrame (frame=0xf5fffdc8, inlineDepth=2, fp=0xf5fffeb0 "\324L\232AB\020", act=0xffffbbe0, cx=0xf6e1d800) at js/src/jit/BaselineBailouts.cpp:1816
#1  js::jit::FinishBailoutToBaseline (bailoutInfo=0x0) at js/src/jit/BaselineBailouts.cpp:1974
#2  0x08544e50 in js::jit::Simulator::softwareInterrupt (this=0xf6e58000, instr=0xf5a60514) at js/src/jit/arm/Simulator-arm.cpp:2587
[...]
#14 0x08581769 in JS_CallFunction (cx=0xf6e1d800, obj=..., fun=..., args=..., rval=...) at js/src/jsapi.cpp:2991
#15 0x084994e2 in OOMTest (cx=0xf6e1d800, argc=1, vp=0xf5a4c060) at js/src/builtin/TestingFunctions.cpp:1650
#16 0x0818ea29 in js::CallJSNative (cx=0xf6e1d800, native=0x84991c0 <OOMTest(JSContext*, unsigned int, JS::Value*)>, args=...) at js/src/jscntxtinlines.h:291
[...]
#30 main (argc=4, argv=0xffffce24, envp=0xffffce38) at js/src/shell/js.cpp:8987
eax	0x0	0
ebx	0xf5d723d0	-170449968
ecx	0xf7da4864	-136689564
edx	0x0	0
esi	0xf5fffdc8	-167772728
edi	0x8d73ff4	148324340
ebp	0xffffb958	4294949208
esp	0xffffb870	4294948976
eip	0x8ab1b7f <js::jit::FinishBailoutToBaseline(js::jit::BaselineBailoutInfo*)+4015>
=> 0x8ab1b7f <js::jit::FinishBailoutToBaseline(js::jit::BaselineBailoutInfo*)+4015>:	movl   $0x0,0x0
   0x8ab1b89 <js::jit::FinishBailoutToBaseline(js::jit::BaselineBailoutInfo*)+4025>:	ud2

Comment 1

[Jason Orendorff [:jorendorff]]

Nicolas, can you take a look?

Comment 2

[Nicolas B. Pierron [:nbp]]

I can reproduce this issue on Mozilla central, after modifying the test case to add braces to single-statement functions.
The test case does print a lot of 42 before stalling and asserting.

Comment 3

[Nicolas B. Pierron [:nbp]]

I can reproduce this issue on x64 as well.

Comment 4

[Fuzzing Team]

JSBugMon: Cannot process bug: Error: Unsupported architecture "Unspecified" required by bug

Comment 5

[Fuzzing Team]

JSBugMon: Bisection requested, failed due to error: Error: Unsupported architecture "Unspecified" required by bug

Comment 6

[Nicolas B. Pierron [:nbp]]

The following testcase crashes on mozilla-central revision fc194660762d+ (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-stdcxx-compat --disable-profiling --enable-debug --without-intl-api --enable-optimize, run with --fuzzing-safe --cpu-count=2):

try {
  Object.defineProperty(this, "fuzzutils", {
    value:{}
  });
} catch(exc) {}
gcPreserveCode()
try {
interruptTest(new Function(""));
oomTest(new Function(`
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;
    for (var i = 0; i < upCount; i++) {
      if (!frame.older)
        break;
      frame = frame.older;
    }
    var completion = frame.eval(code);
    if (completion.return) {
      var v = completion.return;
      if (typeof v === "object")
        v = v.unsafeDereference();
      return v;
    }
    if (completion.throw) {
      var v = completion.throw;
      if (typeof v === "object")
        v = v.unsafeDereference();
      throw v;
    }
    if (completion === null)
      terminate();
  };
})(this);
function f() { evalInFrame(1, "print(a)"); }
function g() { evalInFrame(1, "a = 42"); }
function testGet() { (function ( v1, ... get) { f(); })(); }
function testSet() { (function () { g(); })(); }
try {
  testGet();
} catch (e) {}
try {
  testSet();
} catch (e) {}
`));
Array.prototype.set = function(index, value) {}
} catch(exc) {}

Comment 7

[Nicolas B. Pierron [:nbp]]

The problem comes from an OOM after the creation of the rematerialized frame under js::Debugger::getNewestFrame.

The OOM error is propagated and unwind the caller frame.  Therefore we should catch these errors and clean-up the previously rematerialized frame of the activation such that the top pointer cannot alias a newly created frame.

I should be able to come-up with a patch soonish.

Comment 8

[Nicolas B. Pierron [:nbp]]

I have a patch to fix this assertion, but now I am investigating another assertion which is preventing me from landing this patch.

Comment 9

[Nicolas B. Pierron [:nbp]]

Attachment: Remove rematerialized frames after bailouts and exceptions.

This patch ensure that rematerialized frames used by the Debugger are properly
removed to ensure that no stale data is used during bailouts.

Comment 10

[Jan de Mooij [:jandem]]

Comment on attachment 8981184 [details] [diff] [review]
Remove rematerialized frames after bailouts and exceptions.

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

Good find. Please add a jit-test for this.

::: js/src/jit/BaselineBailouts.cpp
@@ +1950,5 @@
>                  // even if earlier ones failed, to invoke the proper frame
>                  // cleanup in the Debugger.
>                  ok = CopyFromRematerializedFrame(cx, act, outerFp, --inlineDepth,
> +                                                 iter.baselineFrame())
> +                     && ok;

Nit: I think this would be clearer as:

if (!CopyFromRematerializedFrame(...))
    ok = false;

Comment 11

[Nicolas B. Pierron [:nbp]]

(waiting on the second patch from Bug 1464829 to land)

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/66ee3a9fe9ca
Remove rematerialized frames after bailouts and exceptions. r=jandem

Comment 13

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/66ee3a9fe9ca

Comment 14

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Marking this tracking 62 as a reminder that we may want to uplift for beta 10 or 11.

Comment 15

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8981184 [details] [diff] [review]
Remove rematerialized frames after bailouts and exceptions.

Approval Request Comment
[Feature/Bug causing the regression]: …?
[User impact if declined]: bad debugger output.
[Is this code covered by automated tests?]: test case not included as it takes too long with the oomTest function, and hard to synthesized one.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]:  no.
[List of other uplifts needed for the feature/fix]: Bug 1464829 patch*es*
[Is the change risky?]: I do not think so.
[Why is the change risky/not risky?]: Add extra clean-up code to avoid stale values.
[String changes made/needed]: none.

Comment 17

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

We missed the 2nd patch in the other bug initially, so this will need to wait a day or two for uplift. But I haven't forgotten it :)

Comment 18

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8981184 [details] [diff] [review]
Remove rematerialized frames after bailouts and exceptions.

Fix for assertion, the dependent work has all landed on beta so this is ready for uplift (for beta 13)

Comment 19

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/releases/mozilla-beta/rev/c2571cc8a086

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1554e5028374
Add Bug 1363233 test case. r=me

Comment 21

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/1554e5028374