717417 - [jsdbg2] Assertion failure: isFunctionFrame(), at js/src/vm/Stack-inl.h:75

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following code asserts on mozilla-central (revision 4de07a341aab, options -n -m -a):


var lfcode = new Array();
lfcode.push("\
var g = newGlobal('new-compartment');\
var dbg = Debugger(g);\
dbg.onDebuggerStatement = function (frame) {\
    assertEq(frame.eval(\"f();\"), null);\
};\
dbg.onExceptionUnwind = function (frame, exc) {\
    log += 'u';\
};\
g.eval('debugger;');\
");
lfcode.push("var g = newGlobal('new-compartment');\
function LocalTZA() {}\
function LocalTime( t ) {\
  return ( t + LocalTZA() + DaylightSavingTA(t) );\
}\
function DaylightSavingTA( t ) {\
  var dst_start = GetDSTStart(t);\
}\
function GetFirstSundayInMonth( t, m ) {}\
function GetDSTStart( t ) {\
  return (GetFirstSundayInMonth(t, 2) + 7*msPerDay + 2*msPerHour - LocalTZA());\
}\
addNewTestCase(\
  'TDATE = new Date(0); TDATE.setMonth(1,1,1,1,1,1); TDATE',\
  UTCDateFromTime(SetMonth(0,1,1)),\
  LocalDateFromTime(SetMonth(0,1,1)) );\
function addNewTestCase( DateString, UTCDate, LocalDate) {}\
function UTCDateFromTime(t) {}\
function SetMonth( t, mon, date ) {\
  var TIME = LocalTime(t);\
}\
");
evaluate(lfcode[0]);
evaluate(lfcode[1]);

Comment 1

[Jim Blandy :jimb]

Reproduced.

Comment 2

[Jim Blandy :jimb]

Reduced:

var g = newGlobal('new-compartment');
var dbg = Debugger(g);
var frame;
dbg.onExceptionUnwind = function (f, x) {
    frame = f;
    assertEq(frame.live, true);
    throw 'unhandled';
};
dbg.onDebuggerStatement = function(f) {
    assertEq(f.eval('throw 42'), null);
    assertEq(frame.live, false);
};
g.eval('debugger');

It seems like an unhandled exception from onExceptionUnwind can cause us to skip a ScriptDebugEpilogue.

Comment 3

[Jim Blandy :jimb]

Attachment: Don't skip ScriptDebugEpilogue when an onExceptionUnwind handler throws an uncaught exception or terminates the debuggee.

One-line fix, 33 lines of tests. Great day. :)

Comment 4

[Jason Orendorff [:jorendorff]]

Comment on attachment 588309 [details] [diff] [review]
Don't skip ScriptDebugEpilogue when an onExceptionUnwind handler throws an uncaught exception or terminates the debuggee.

I don't understand this code very well, but I don't think this is quite right. I think you also need:

>+      if (cx->isExceptionPending()) {
>         pc = FindExceptionHandler(cx);
>         if (pc)
>             break;
>+      }

with appropriate re-indentation, of course, and an appropriate test.

These tests will fail to produce the desired situation if we ever change onExceptionUnwind error handling to match the spec (throw to the debuggee rather than terminate it) -- which I would really like to do!

So, though I hate to add more work on this fairly simple patch, I would appreciate tests that trigger the same behavior in a different way. You could use another Debugger to terminate the onExceptionUnwind hook-- but a more direct approach would be to add a terminate() function to the shell.

Clearing review flag.

Comment 5

[Jim Blandy :jimb]

Unless there's a pre-existing bug, I think it must be correct as written:

If a stub makes some call that is terminated, then the stub will call THROW(), and control will reach js_InternalThrow with no exception set. The whole call to Debugger::onExceptionUnwind will be skipped, and control then reaches the FindExceptionHandler call you mention. Since that's the path normally taken for termination, it must be okay if a termination caused by onExceptionUnwind takes that same path.

Oh, here's the answer: FindExceptionHandler checks cx->isExceptionPending.

Comment 6

[Jim Blandy :jimb]

I think a 'terminate' function in the shell would be awesome.

Comment 7

[Jim Blandy :jimb]

(In reply to Jason Orendorff [:jorendorff] from comment #4)
> These tests will fail to produce the desired situation if we ever change
> onExceptionUnwind error handling to match the spec (throw to the debuggee
> rather than terminate it) -- which I would really like to do!

Are you confusing onExceptionUnwind and handleUncaughtException? In onExceptionUnwind-09.js, the onExceptionUnwind handler explicitly requests termination, which won't be affected by any improvements to the unhandled exception handling (now that's one that should be returned to the State Home For Unfortunate Noun Clauses).

Comment 8

[Jim Blandy :jimb]

Attachment: Add 'terminate' shell primitive.

Comment 9

[Jim Blandy :jimb]

Attachment: Don't skip ScriptDebugEpilogue when an onExceptionUnwind handler throws an uncaught exception or terminates the debuggee.

Added test in which the onExceptionUnwind handler is explicitly terminated.

Comment 10

[Jason Orendorff [:jorendorff]]

Comment on attachment 588656 [details] [diff] [review]
Add 'terminate' shell primitive.

OK. It's Really Too Bad that this results in a 0 exit code from the shell. Is that bug on file?

Comment 11

[Jason Orendorff [:jorendorff]]

Comment on attachment 588657 [details] [diff] [review]
Don't skip ScriptDebugEpilogue when an onExceptionUnwind handler throws an uncaught exception or terminates the debuggee.

I misread onExceptionUnwind-09. Oops. Yeah, this is great.

Comment 12

[Jim Blandy :jimb]

(In reply to Jason Orendorff [:jorendorff] from comment #10)
> OK. It's Really Too Bad that this results in a 0 exit code from the shell.
> Is that bug on file?

Filed, bug 718786.

Comment 13

[Jim Blandy :jimb]

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=650f4fa9ca12

Comment 14

[Jim Blandy :jimb]

Try server run: https://tbpl.mozilla.org/?tree=Try&rev=3d5d769f323e

Comment 15

[Jim Blandy :jimb]

Thanks, bugzilla.

Comment 16

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/93602e4ac4d9

Comment 17

[Marco Bonardo [:mak]]

and https://hg.mozilla.org/mozilla-central/rev/650f4fa9ca12