Last Comment Bug 717417 - [jsdbg2] Assertion failure: isFunctionFrame(), at js/src/vm/Stack-inl.h:75
: [jsdbg2] Assertion failure: isFunctionFrame(), at js/src/vm/Stack-inl.h:75
Status: RESOLVED FIXED
: assertion, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- critical (vote)
: mozilla12
Assigned To: Jim Blandy :jimb
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on: 678364
Blocks: langfuzz
  Show dependency treegraph
 
Reported: 2012-01-11 14:28 PST by Christian Holler (:decoder)
Modified: 2012-01-24 05:10 PST (History)
5 users (show)
jimb: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Don't skip ScriptDebugEpilogue when an onExceptionUnwind handler throws an uncaught exception or terminates the debuggee. (2.07 KB, patch)
2012-01-12 21:42 PST, Jim Blandy :jimb
no flags Details | Diff | Splinter Review
Add 'terminate' shell primitive. (1.81 KB, patch)
2012-01-14 09:54 PST, Jim Blandy :jimb
jorendorff: review+
Details | Diff | Splinter Review
Don't skip ScriptDebugEpilogue when an onExceptionUnwind handler throws an uncaught exception or terminates the debuggee. (2.74 KB, patch)
2012-01-14 09:55 PST, Jim Blandy :jimb
jorendorff: review+
Details | Diff | Splinter Review

Description Christian Holler (:decoder) 2012-01-11 14:28:50 PST
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 2012-01-12 16:41:43 PST
Reproduced.
Comment 2 Jim Blandy :jimb 2012-01-12 21:22:25 PST
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 2012-01-12 21:42:17 PST
Created attachment 588309 [details] [diff] [review]
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] 2012-01-13 13:27:46 PST
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 2012-01-13 18:21:25 PST
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 2012-01-13 18:21:58 PST
I think a 'terminate' function in the shell would be awesome.
Comment 7 Jim Blandy :jimb 2012-01-13 18:25:46 PST
(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 2012-01-14 09:54:06 PST
Created attachment 588656 [details] [diff] [review]
Add 'terminate' shell primitive.
Comment 9 Jim Blandy :jimb 2012-01-14 09:55:09 PST
Created attachment 588657 [details] [diff] [review]
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] 2012-01-16 12:15:18 PST
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] 2012-01-16 12:19:53 PST
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 2012-01-17 12:31:58 PST
(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 14 Jim Blandy :jimb 2012-01-23 17:51:14 PST
Try server run: https://tbpl.mozilla.org/?tree=Try&rev=3d5d769f323e
Comment 15 Jim Blandy :jimb 2012-01-23 17:52:11 PST
Thanks, bugzilla.
Comment 16 Marco Bonardo [::mak] 2012-01-24 05:09:20 PST
https://hg.mozilla.org/mozilla-central/rev/93602e4ac4d9
Comment 17 Marco Bonardo [::mak] 2012-01-24 05:09:54 PST
and https://hg.mozilla.org/mozilla-central/rev/650f4fa9ca12

Note You need to log in before you can comment on or make changes to this bug.