1124121 - crash in PatchBaselineFramesForDebugMode

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Jan de Mooij [:jandem]]

This bug was filed from the Socorro interface and is 
report bp-e6c4d34a-98c0-499e-a1e3-de7ed2150120.
=============================================================

We have a crash in PatchBaselineFramesForDebugMode on 37 and 38. It's #26 for 37.

It's the ICEntry::Kind_DebugPrologue case and we're crashing here:

  recompInfo->resumeAddr = bl->postDebugPrologueAddr();

CheckOverRecursedWithExtra is on the stack; we seem to be handling an interrupt and that's toggling the debugger somehow.

Comment 1

[Jan de Mooij [:jandem]]

[Tracking Requested - why for this release]:
Debugger crashes.

Comment 2

[Jan de Mooij [:jandem]]

Here's a full stack:

https://crash-stats.mozilla.com/report/index/e6c4d34a-98c0-499e-a1e3-de7ed2150120

What I don't understand is that we have a Kind_DebugPrologue entry but don't have a DebugPrologue frame on the stack. Maybe we need the CheckOverRecursedWithExtra entry and instead get the DebugPrologue one?

Comment 3

[Jan de Mooij [:jandem]]

Here we go:

var global = this;
setInterruptCallback(function() {
    print("Interrupt!");
    var g = newGlobal();
    g.parent = global;
    g.eval("var dbg = new Debugger(parent); dbg.onEnterFrame = function(frame) {};");
    return true;
});

function f(x) {
    if (x > 200)
	return;
    interruptIf(x == 100);
    f(x + 1);
}
f(0);

Assertion failure: needsRecompileInfo() && recompInfo, at jit/BaselineDebugModeOSR.cpp:112

Comment 4

[Shu-yu Guo [:shu]]

Attachment: Handle the stack check VM call in debug mode OSR.

I don't see a cleaner way around introducing new ICEntry types for now.

As a cleanup I'll try to make the debug prologue/epilogue offsets tomorrow fake
ICEntries as well tomorrow, instead of tracking their offsets manually.

Comment 5

[Shu-yu Guo [:shu]]

Great job coming up with the test case, by the way. Much appreciated!

Comment 6

[Jan de Mooij [:jandem]]

Comment on attachment 8552411 [details] [diff] [review]
Handle the stack check VM call in debug mode OSR.

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

Thanks for fixing this so quickly, looks good.

::: js/src/jit/BaselineIC.h
@@ +295,5 @@
>      }
>  
>      Kind kind() const {
>          // MSVC compiles enums as signed.
> +        return (Kind)(kind_ & 0xf);

Nit: Kind(kind_ & 0xf); while you're here.

Comment 7

[Shu-yu Guo [:shu]]

Attachment: Handle the stack check VM calls in debug mode OSR.

Did some cleanup.

I had hoped to get rid of postDebugPrologueOffset and epilogueOffset and handle
all of them as a return-from-callVM. It's possible to use the ICEntry for the
stack checks because those callVMs are always generated. If debug
instrumentation isn't enabled though, the debug prologue and epilogue stuff
won't actually be return-from-callVM, and would need special handling anyways.
I ended up leaving those as is.

Comment 8

[Jan de Mooij [:jandem]]

Comment on attachment 8552818 [details] [diff] [review]
Handle the stack check VM calls in debug mode OSR.

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

Very nice, thanks.

::: js/src/jit-test/tests/debug/execution-observability-04.js
@@ +4,5 @@
> +setInterruptCallback(function() {
> +  print("Interrupt!");
> +  var g = newGlobal();
> +  g.parent = global;
> +  g.eval("var dbg = new Debugger(parent); dbg.onEnterFrame = function(frame) {};");

Nit: can you increment a counter (on 'parent' maybe) in onEnterFrame and check its value at the end of the test? Just to ensure we really called the interrupt handler and recompiled the script.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

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

Comment 10

[Lawrence Mandel [:lmandel] (use needinfo)]

The fix has been on m-c for a few days. 37 is marked as affected. Do you want to request uplift to Aurora?

Also, I am unclear from the comments whether this issue is limited to debug builds or impacts all builds.

Comment 11

[Shu-yu Guo [:shu]]

(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #10)
> The fix has been on m-c for a few days. 37 is marked as affected. Do you
> want to request uplift to Aurora?
> 
> Also, I am unclear from the comments whether this issue is limited to debug
> builds or impacts all builds.

The bug impacts both optimized and debug builds. The confusingly named "debug mode" here refers to debugging JS (like from our builtin devtools Debugger).

I'll request uplift.

Comment 12

[Shu-yu Guo [:shu]]

Comment on attachment 8552818 [details] [diff] [review]
Handle the stack check VM calls in debug mode OSR.

Approval Request Comment
[Feature/regressing bug #]: 1032869
[User impact if declined]: Crashes in cases where the user attempts to do slow-script debugging while the code is stuck in recursion instead of an infinite loop.
[Describe test coverage new/current, TreeHerder]: on central
[Risks and why]: Low. No behavior change, bug fix.
[String/UUID change made/needed]: None.

Comment 13

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8552818 [details] [diff] [review]
Handle the stack check VM calls in debug mode OSR.

Aurora+

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e3367427f25b