Closed Bug 717417 Opened 10 years ago Closed 10 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla12

People

(Reporter: decoder, Assigned: jimb)

References

Details

(Keywords: assertion, testcase)

Attachments

(2 files, 1 obsolete file)

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]);
Reproduced.
Assignee: jorendorff → 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.
One-line fix, 33 lines of tests. Great day. :)
Attachment #588309 - Flags: review?(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.
Attachment #588309 - Flags: review?(jorendorff)
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.
I think a 'terminate' function in the shell would be awesome.
(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).
Attachment #588656 - Flags: review?(jorendorff)
Added test in which the onExceptionUnwind handler is explicitly terminated.
Attachment #588309 - Attachment is obsolete: true
Attachment #588657 - Flags: review?(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?
Attachment #588656 - Flags: review?(jorendorff) → review+
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.
Attachment #588657 - Flags: review?(jorendorff) → review+
(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.
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=650f4fa9ca12
Assignee: jimb → blackconnect
Status: NEW → ASSIGNED
Component: JavaScript Engine → Java-Implemented Plugins
Flags: in-testsuite+
OS: Linux → All
QA Contact: general → blackconnect
Hardware: x86_64 → All
Target Milestone: --- → mozilla13
Thanks, bugzilla.
Assignee: blackconnect → general
Component: Java-Implemented Plugins → JavaScript Engine
QA Contact: blackconnect → general
Assignee: general → jimb
https://hg.mozilla.org/mozilla-central/rev/93602e4ac4d9
Target Milestone: mozilla13 → mozilla12
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.