Bug 1601171 - Part 3: Run all debugger hooks, even if some throw uncaught exceptions. r=jimb

We want each hook to run in sequence, and we do not want the failure of one
hook to cause later hooks to skip execution. Doing so would make it difficult
for consumers of the Debugger API to be confident that their hooks will be
consistently executed, since entirely independent code could otherwise cause
their logic to be silently skipped.

Differential Revision: https://phabricator.services.mozilla.com/D58186

--HG--
extra : moz-landing-system : lando

js/src/debugger/Debugger.cpp

@@ -836,8 +836,7 @@ bool DebuggerList<HookIsEnabledFun>::dispatchResumeModeHook(
     bool result = dbg->enterDebuggerHook(
         cx, [&]() -> bool { return fireHook(dbg, resumeMode, rval); });
     if (!result) {
-      resumeMode = ResumeMode::Terminate;
-      rval.setUndefined();
+      return false;
     }
 
     // Stop execution at the first non-Continue hook.
@@ -1156,7 +1155,7 @@ bool DebugAPI::slowPathOnLeaveFrame(JSContext* cx, AbstractFramePtr frame,
           adjqi.runJobs();
 
           if (!result) {
-            resumeMode = ResumeMode::Terminate;
+            return false;
           }
 
           // At this point, we are back in the debuggee compartment, and
@@ -2515,7 +2514,7 @@ bool DebugAPI::onTrap(JSContext* cx) {
         adjqi.runJobs();
 
         if (!result) {
-          resumeMode = ResumeMode::Terminate;
+          return false;
         }
 
         if (resumeMode != ResumeMode::Continue) {
@@ -2649,7 +2648,7 @@ bool DebugAPI::onSingleStep(JSContext* cx) {
       adjqi.runJobs();
 
       if (!result) {
-        resumeMode = ResumeMode::Terminate;
+        return false;
       }
 
       if (resumeMode != ResumeMode::Continue) {
@@ -2755,7 +2754,11 @@ void DebugAPI::slowPathOnNewGlobalObject(JSContext* cx,
       adjqi.runJobs();
 
       if (!result) {
-        resumeMode = ResumeMode::Terminate;
+        // Like other quiet hooks using dispatchQuietHook, this hook
+        // silently ignores all errors that propagate out of it and aren't
+        // already handled by the hook error reporting.
+        cx->clearPendingException();
+        break;
       }
       if (resumeMode != ResumeMode::Continue &&
           resumeMode != ResumeMode::Return) {

js/src/debugger/Debugger.h

@@ -989,13 +989,18 @@ class Debugger : private mozilla::LinkedListElement<Debugger> {
   MOZ_MUST_USE bool enterDebuggerHook(JSContext* cx, RunImpl runImpl) {
     AutoRealm ar(cx, object);
 
-    bool ok = true;
     if (!runImpl()) {
+      // We do not want errors within one hook to effect errors in other hooks,
+      // so the only errors that we allow to propagate out of a debugger hook
+      // are OOM errors and general terminations.
+      if (!cx->isExceptionPending() || cx->isThrowingOutOfMemory()) {
+        return false;
+      }
+
       reportUncaughtException(cx);
-      ok = false;
     }
     MOZ_ASSERT(!cx->isExceptionPending());
-    return ok;
+    return true;
   }
 
   MOZ_MUST_USE bool fireDebuggerStatement(JSContext* cx, ResumeMode& resumeMode,

js/src/jit-test/tests/debug/bug1130756.js

@@ -1,28 +0,0 @@
-// |jit-test| error: timeout
-
-options('werror');
-
-var g = newGlobal({newCompartment: true});
-g.parent = this;
-g.eval("(" + function() {
-var dbg = Debugger(parent);
-var handler = {hit: function() {}};
-
-dbg.onEnterFrame = function(frame) {
-    frame.onStep = function() {}
-}
-} + ")()");
-
-g = newGlobal({newCompartment: true});
-g.parent = this;
-g.eval("Debugger(parent).onExceptionUnwind = function () {};");
-
-function f(x) {
-    if (x === 0) {
-        return;
-    }
-    f(x - 1);
-    f(x - 1);
-}
-timeout(0.00001);
-f(100);

js/src/jit-test/tests/debug/onEnterFrame-04.js

@@ -1,50 +0,0 @@
-// We detect and stop the runaway recursion caused by making onEnterFrame a
-// wrapper of a debuggee function.
-
-// This is all a bit silly. In any reasonable design, both debugger re-entry
-// (the second onEnterFrame invocation) and debuggee re-entry (the call to g.f
-// from within the debugger, not via a Debugger invocation function) would raise
-// errors immediately. We have plans to do so, but in the mean time, we settle
-// for at least detecting the recursion.
-
-load(libdir + 'asserts.js');
-
-var g = newGlobal({newCompartment: true});
-g.eval("function f(frame) { n++; return 42; }");
-g.n = 0;
-
-var dbg = Debugger();
-var gw = dbg.addDebuggee(g);
-
-// Register the debuggee function as the onEnterFrame handler. When we first
-// call or eval in the debuggee:
-//
-// - The onEnterFrame call reporting that frame's creation is itself an event
-//   that must be reported, so we call onEnterFrame again.
-//
-// - SpiderMonkey detects the out-of-control recursion, and generates a "too
-//   much recursion" InternalError in the youngest onEnterFrame call.
-//
-// - We don't catch it, so the onEnterFrame handler call itself throws.
-//
-// - Since the Debugger doesn't have an uncaughtExceptionHook (it can't; such a
-//   hook would itself raise a "too much recursion" exception), Spidermonkey
-//   reports the exception immediately and terminates the debuggee --- which is
-//   the next-older onEnterFrame call.
-//
-// - This termination propagates all the way out to the initial attempt to
-//   create a frame in the debuggee.
-dbg.onEnterFrame = g.f;
-
-// Get a Debugger.Object instance referring to f.
-var debuggeeF = gw.makeDebuggeeValue(g.f);
-
-// Using f.call allows us to catch the termination.
-assertEq(debuggeeF.call(), null);
-
-// We should never actually begin execution of the function.
-assertEq(g.n, 0);
-
-// When an error is reported, the shell usually exits with a nonzero exit code.
-// If we get here, the test passed, so override that behavior.
-quit(0);

js/src/jit-test/tests/debug/onExceptionUnwind-08.js

@@ -1,5 +1,5 @@
-// Ensure that ScriptDebugEpilogue gets called when onExceptionUnwind
-// throws an uncaught exception.
+// Ensure that uncaught exceptions thrown in onExceptionUnwind do not
+// propagate outward into debuggee execution.
 var g = newGlobal({newCompartment: true});
 var dbg = Debugger(g);
 var frame;
@@ -9,7 +9,7 @@ dbg.onExceptionUnwind = function (f, x) {
     throw 'unhandled';
 };
 dbg.onDebuggerStatement = function(f) {
-    assertEq(f.eval('throw 42'), null);
+    assertEq(f.eval('throw 42').throw, 42);
     assertEq(frame.onStack, false);
 };
 g.eval('debugger');