Closed Bug 1437537 Opened 6 years ago Closed 6 years ago

Assertion failure: !cx->isExceptionPending(), at js/src/vm/Debugger.cpp:1460

Categories

(Core :: JavaScript Engine, defect, P2)

Product:
Core
Component:
JavaScript Engine
Platform:
x86_64
Linux
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox59 --- wontfix
firefox60 --- wontfix
firefox61 --- fixed

People

(Reporter: decoder, Assigned: jorendorff)

Details

(4 keywords, Whiteboard: [jsbugmon:update])

Attachments

(1 file)

Fix assertion when forcing a return while paused on JSOP_EXCEPTION
4.28 KB, patch
jimb
: review+
Details | Diff | Splinter Review 
The following testcase crashes on mozilla-central revision 2b7d42d527af (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --disable-profiling --enable-debug --enable-optimize, run with --fuzzing-safe --ion-offthread-compile=off):

var g = newGlobal();
var dbg = new Debugger(g);
dbg.onEnterFrame = function(frame) {
    var step = 0;
    frame.onStep = function() {
        ++step;
        if (step == 3) return {
            return: undefined
        }
    }
}
setJitCompilerOption("ion.warmup.trigger", 30);
b: g.eval("try { g(); } catch (e) { }") + (this) << (this) << 1


Backtrace:

received signal SIGSEGV, Segmentation fault.
0x0000000000b176f0 in js::Debugger::resultToCompletion (cx=0x7ffff5f16000, ok=<optimized out>, rv=..., status=0x7fffffffb558, value=...) at js/src/vm/Debugger.cpp:1460
#0  0x0000000000b176f0 in js::Debugger::resultToCompletion (cx=0x7ffff5f16000, ok=<optimized out>, rv=..., status=0x7fffffffb558, value=...) at js/src/vm/Debugger.cpp:1460
#1  0x0000000000b51686 in js::Debugger::slowPathOnLeaveFrame (cx=cx@entry=0x7ffff5f16000, frame=..., pc=pc@entry=0x7ffff49f985c "\346\001\230\216\213", frameOk=frameOk@entry=true) at js/src/vm/Debugger.cpp:957
#2  0x0000000000574e03 in js::Debugger::onLeaveFrame (cx=cx@entry=0x7ffff5f16000, frame=..., pc=pc@entry=0x7ffff49f985c "\346\001\230\216\213", ok=ok@entry=true) at js/src/vm/Debugger-inl.h:24
#3  0x00000000005579f7 in ForcedReturn (frameOk=true, ei=..., regs=..., cx=0x7ffff5f16000) at js/src/vm/Interpreter.cpp:1156
#4  ForcedReturn (cx=0x7ffff5f16000, regs=...) at js/src/vm/Interpreter.cpp:1167
#5  0x000000000056bcdf in Interpret (cx=0x7ffff5f16000, state=...) at js/src/vm/Interpreter.cpp:1966
#6  0x000000000056c965 in js::RunScript (cx=0x7ffff5f16000, state=...) at js/src/vm/Interpreter.cpp:423
#7  0x000000000056fb8d in js::ExecuteKernel (cx=0x7ffff5f16000, script=..., script@entry=..., envChainArg=..., newTargetValue=..., evalInFrame=..., evalInFrame@entry=..., result=<optimized out>) at js/src/vm/Interpreter.cpp:706
#8  0x00000000005a28b4 in EvalKernel (cx=0x7ffff5f16000, v=..., evalType=evalType@entry=INDIRECT_EVAL, caller=..., env=..., pc=pc@entry=0x0, vp=...) at js/src/builtin/Eval.cpp:324
#9  0x00000000005a2ca0 in js::IndirectEval (cx=0x7ffff5f16000, argc=<optimized out>, vp=<optimized out>) at js/src/builtin/Eval.cpp:417
#10 0x0000000000578861 in js::CallJSNative (cx=0x7ffff5f16000, native=0x5a2bb0 <js::IndirectEval(JSContext*, unsigned int, JS::Value*)>, args=...) at js/src/jscntxtinlines.h:291
#11 0x000000000056ce57 in js::InternalCallOrConstruct (cx=cx@entry=0x7ffff5f16000, args=..., construct=construct@entry=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:473
#12 0x000000000056d22d in InternalCall (cx=cx@entry=0x7ffff5f16000, args=...) at js/src/vm/Interpreter.cpp:522
#13 0x000000000056d3b0 in js::Call (cx=cx@entry=0x7ffff5f16000, fval=..., fval@entry=..., thisv=..., args=..., rval=...) at js/src/vm/Interpreter.cpp:541
#14 0x0000000000aecd27 in js::ForwardingProxyHandler::call (this=this@entry=0x2044ea0 <js::CrossCompartmentWrapper::singleton>, cx=0x7ffff5f16000, proxy=..., proxy@entry=..., args=...) at js/src/proxy/Wrapper.cpp:176
#15 0x0000000000ac8f7b in js::CrossCompartmentWrapper::call (this=0x2044ea0 <js::CrossCompartmentWrapper::singleton>, cx=0x7ffff5f16000, wrapper=..., args=...) at js/src/proxy/CrossCompartmentWrapper.cpp:359
#16 0x0000000000ac3855 in js::Proxy::call (cx=0x7ffff5f16000, proxy=proxy@entry=..., args=...) at js/src/proxy/Proxy.cpp:511
#17 0x0000000000ac391d in js::proxy_Call (cx=0x7ffff5f16000, argc=<optimized out>, vp=<optimized out>) at js/src/proxy/Proxy.cpp:770
#18 0x0000000000578861 in js::CallJSNative (cx=0x7ffff5f16000, native=0xac38a0 <js::proxy_Call(JSContext*, unsigned int, JS::Value*)>, args=...) at js/src/jscntxtinlines.h:291
[...]
#32 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at js/src/shell/js.cpp:9317
rax	0x0	0
rbx	0x7ffff5f16000	140737319624704
rcx	0x7ffff6c282ad	140737333330605
rdx	0x0	0
rsi	0x7ffff6ef7770	140737336276848
rdi	0x7ffff6ef6540	140737336272192
rbp	0x7fffffffb4c0	140737488336064
rsp	0x7fffffffb4a0	140737488336032
r8	0x7ffff6ef7770	140737336276848
r9	0x7ffff7fe4780	140737354024832
r10	0x58	88
r11	0x7ffff6b9e7a0	140737332766624
r12	0x7fffffffb558	140737488336216
r13	0x0	0
r14	0x1	1
r15	0x7fffffffb5a0	140737488336288
rip	0xb176f0 <js::Debugger::resultToCompletion(JSContext*, bool, JS::Value const&, JSTrapStatus*, JS::MutableHandle<JS::Value>)+256>
=> 0xb176f0 <js::Debugger::resultToCompletion(JSContext*, bool, JS::Value const&, JSTrapStatus*, JS::MutableHandle<JS::Value>)+256>:	movl   $0x0,0x0
   0xb176fb <js::Debugger::resultToCompletion(JSContext*, bool, JS::Value const&, JSTrapStatus*, JS::MutableHandle<JS::Value>)+267>:	ud2
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/5b77f711e2b4
user:        Shu-yu Guo
date:        Wed Jun 28 14:22:00 2017 -0400
summary:     Bug 1375447 - Save exception state in Debugger::onTrap. r=jandem

This iteration took 256.398 seconds to run.
Jan, not sure who should own this now? Or :jimb?
Flags: needinfo?(jdemooij)
I'll take a look tomorrow.
This is a debugger issue. What's happening is:

(1) The interpreter throws an exception under GetNameOperation.

(2) We enter a catch block, then we end up here:

https://searchfox.org/mozilla-central/rev/769222fadff46164f8cc0dc7a0bae5a60dc2f335/js/src/vm/Interpreter.cpp#1954

(3) We call the onStep handler and do a forced return.

(4) We end up in Debugger::slowPathOnLeaveFrame. We assert in Debugger::resultToCompletion, because we have ok == true but we still have the pending exception on the context.

We should probably clear the exception somewhere.
Flags: needinfo?(jdemooij) → needinfo?(jimb)
I agree: if Debugger forces a return from a catch handler, then the exception is considered caught, so it should have been cleared.
Flags: needinfo?(jimb)
Is the source location of the frame passed to the onStep handler (as its `this` value) in the catch handler? Comment 4 suggests this is so; I think that means that the exception should have been cleared before step (3).
No, it shouldn't. Below is what the bytecode for a try-catch block looks like; I'm going to bet the frame's offset is pointing at something like the instructions 20-29, since it's only the JSOP_EXCEPTION bytecode that actually transfers the exception from the context (clearing it) onto the stack.

We were clearly aware that this case could arise when we wrote the onstep handler, because we carefully save the exception across handler calls, and explain why:
https://searchfox.org/mozilla-central/rev/769222fadff46164f8cc0dc7a0bae5a60dc2f335/js/src/vm/Debugger.cpp#2064-2070

But perhaps we didn't try doing a forced return?

--- bytecode for a try/catch block:

js> dis(() => { try { throw new Error(); } catch (e) { print(e); } })
flags: LAMBDA ARROW
loc     op
-----   --
main:
00000:  try 20 (+20)                    # 
00001:  getgname "Error"                # Error
00006:  is-constructing                 # Error JS_IS_CONSTRUCTING
00007:  dupat 1                         # Error JS_IS_CONSTRUCTING Error
00011:  new 0                           # (new Error(...))
00014:  throw                           # 
00015:  goto 55 (+40)                   # !!! UNREACHABLE !!!

# try-catch from try @ 00000
00020:  jumptarget                      # 
00021:  undefined                       # undefined
00022:  setrval                         # 
00023:  uninitialized                   # UNINITIALIZED
00024:  initlexical 0                   # UNINITIALIZED
00028:  pop                             # 
00029:  exception                       # EXCEPTION
00030:  initlexical 0                   # EXCEPTION
00034:  pop                             # 
00035:  getgname "print"                # print
00040:  gimplicitthis "print"           # print THIS
00045:  getlocal 0                      # print THIS e
00049:  call-ignores-rv 1               # print(...)
00052:  pop                             # 
00053:  debugleavelexicalenv            # 
00054:  nop                             # 
00055:  jumptarget                      # 
00056:  retrval                         #
Slightly reduced test case, unfortunately still with magic numbers:
var g = newGlobal();
var dbg = new Debugger(g);
dbg.onEnterFrame = function(frame) {
  frame.onStep = function() {
    if (this.offset == 20)
      return { return: undefined };
  }
}
let f = g.Function(`try { throw new Error; } catch (e) { print("caught: " + e); }`);
f();
No magic numbers, fuzzer-like behavior:

var g = newGlobal();
var dbg = new Debugger(g);
let f = g.Function(`try { throw new Error; } catch (e) { return 'natural'; }`);

let limit = -1;
for(;;) {
  dbg.onEnterFrame = function(frame) {
    frame.onStep = function() {
      if (this.offset > limit) {
        limit = this.offset;
        return { return: 'forced' };
      }
    }
  }
  if (f() == 'natural')
    break;
}
Priority: -- → P1
Priority: P1 → P2
Assignee: nobody → jorendorff
Status: NEW → ASSIGNED
Comment on attachment 8957036 [details] [diff] [review]
Fix assertion when forcing a return while paused on JSOP_EXCEPTION

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

Fantastic.
Attachment #8957036 - Flags: review?(jimb) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/c2d7e417c6fbf4fe6d5584be183f6e0f10c0a524
Bug 1437537 - Fix assertion when forcing a return while paused on JSOP_EXCEPTION. r=jimb.
Whiteboard: [jsbugmon:update] → [jsbugmon:update,ignore]
JSBugMon: The testcase found in this bug no longer reproduces (tried revision 6d0f84afc194).
https://hg.mozilla.org/mozilla-central/rev/c2d7e417c6fb
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Is there a user impact here which justifies Beta uplift consideration, or can this ride the trains?
status-firefox59: --- → wontfix
status-firefox-esr52: --- → unaffected
Flags: needinfo?(jorendorff)
Flags: in-testsuite+
Whiteboard: [jsbugmon:update,ignore] → [jsbugmon:update]
Re: comment 18, I don't think so. Forwarding ni? to Jim in case he disagrees.
Flags: needinfo?(jorendorff) → needinfo?(jimb)
I think it can ride the trains.
Flags: needinfo?(jimb)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: