The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla12

Status

()

Core
JavaScript Engine
--
critical
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: decoder, Assigned: jimb)

Tracking

(Blocks: 1 bug, {assertion, testcase})

Trunk
mozilla12
assertion, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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]);
(Assignee)

Comment 1

5 years ago
Reproduced.
(Assignee)

Updated

5 years ago
Assignee: jorendorff → jimb
(Assignee)

Comment 2

5 years ago
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.
(Assignee)

Comment 3

5 years ago
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. :)
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)
(Assignee)

Comment 5

5 years ago
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.
(Assignee)

Comment 6

5 years ago
I think a 'terminate' function in the shell would be awesome.
(Assignee)

Comment 7

5 years ago
(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).
(Assignee)

Comment 8

5 years ago
Created attachment 588656 [details] [diff] [review]
Add 'terminate' shell primitive.
Attachment #588656 - Flags: review?(jorendorff)
(Assignee)

Comment 9

5 years ago
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.
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+
(Assignee)

Comment 12

5 years ago
(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.
(Assignee)

Comment 13

5 years ago
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
(Assignee)

Comment 14

5 years ago
Try server run: https://tbpl.mozilla.org/?tree=Try&rev=3d5d769f323e
(Assignee)

Comment 15

5 years ago
Thanks, bugzilla.
Assignee: blackconnect → general
Component: Java-Implemented Plugins → JavaScript Engine
QA Contact: blackconnect → general

Updated

5 years ago
Assignee: general → jimb
https://hg.mozilla.org/mozilla-central/rev/93602e4ac4d9
Target Milestone: mozilla13 → mozilla12
and https://hg.mozilla.org/mozilla-central/rev/650f4fa9ca12

Updated

5 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.