744730 - [jsdbg2] Assertion failure: uint32_t(prevpc_ - prev_->script()->code) < prev_->script()->length, at js/src/vm/Stack-inl.h:108

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following code asserts on mozilla-central revision 3fa30b0edd15 (options -n -m -a):


var g = newGlobal('new-compartment');
var dbg = Debugger(g);
dbg.onDebuggerStatement = function (stack) { return {return: 1234}; };
g.eval("function f() { debugger; }");
g.eval("function g() { f(); }");
g.eval("function h() { g(); }");
g.eval("function i() { h(); }");
function logger(frame, mark) { 
    return function (completion) {
    };
}
dbg.onEnterFrame = function handleEnter(f) {
    f.onPop = logger(f, f.callee.name + ")");  
};
g.i();

Comment 1

[Jason Orendorff [:jorendorff]]

Still occurs (with --no-ion -m -n -a):

var g = newGlobal('new-compartment');
var dbg = Debugger(g);
dbg.onDebuggerStatement = function (f) { return {return: 1234}; };
dbg.onEnterFrame = function (f) {
    f.onPop = function () {};
};
g.eval("debugger;");



onPop hooks are strange. They cause us to re-enter the interpreter from js::ScriptDebugEpilogue.

We re-enter via Invoke, which asserts that the previous frame's prevpc_ is in the right range; in this case the previous frame is g.eval (the one being popped) and prevpc_ is NULL and *prev_->script() is full of garbage.

Comment 2

[Jason Orendorff [:jorendorff]]

Disregard the last paragraph of comment 1. Here's what's really happening.

TrampolineCompiler::generateForceReturn calls masm.fallibleVMCall, passing NULL as the pc.

masm.fallibleVMCall calls setupFallibleVMframe which faithfully emits code to set regs.pc = NULL. In this case, it happens that the value that was already there was correct. I think it always will be, in the forceReturn path.

The "obvious" fix of emitting the code to set regs.pc only if we have a non-null pc to store there seems to work. I'll post the patch Monday. Maybe there's a better way though?

Comment 3

[Jason Orendorff [:jorendorff]]

Attachment: v1

Comment 4

[Jim Blandy :jimb]

Comment on attachment 687749 [details] [diff] [review]
v1

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

This looks good. However, the comments atop fallibleVMCall and its subroutines should be changed to document that a NULL pc value means "current regs can be trusted"; that's part of their interface now, so it should appear there, not in the body.

Thanks for the fix! I'm a little disturbed our existing tests didn't catch this.

Comment 5

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3b2331322084

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3b2331322084