Closed Bug 1124121 Opened 5 years ago Closed 5 years ago

crash in PatchBaselineFramesForDebugMode

Categories

(Core :: JavaScript Engine: JIT, defect, critical)

x86
Windows NT
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox37 + fixed
firefox38 + fixed

People

(Reporter: jandem, Assigned: shu)

Details

(Keywords: crash)

Crash Data

Attachments

(1 file, 1 obsolete file)

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.
[Tracking Requested - why for this release]:
Debugger crashes.
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?
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
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.
Attachment #8552411 - Flags: feedback?(jdemooij)
Great job coming up with the test case, by the way. Much appreciated!
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.
Attachment #8552411 - Flags: feedback?(jdemooij) → feedback+
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.
Attachment #8552411 - Attachment is obsolete: true
Attachment #8552818 - Flags: review?(jdemooij)
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.
Attachment #8552818 - Flags: review?(jdemooij) → review+
https://hg.mozilla.org/mozilla-central/rev/bd9169b924a1
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
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.
Assignee: nobody → shu
Flags: needinfo?(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.
Flags: needinfo?(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.
Attachment #8552818 - Flags: approval-mozilla-aurora?
Comment on attachment 8552818 [details] [diff] [review]
Handle the stack check VM calls in debug mode OSR.

Aurora+
Attachment #8552818 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.