Bug 1602699 - Part 2: Change DebugAPI::onDebuggerStatement force-return to work via an error. r=jimb,jandem
authorLogan Smyth <loganfsmyth@gmail.com>
Tue, 17 Dec 2019 22:27:40 +0000
changeset 507675 96c9ae1624725fa184e7696318efefa373fa8a85
parent 507674 d64b4042eb5a1ba877356ec4bb1642f0cb5f2608
child 507676 0340540c6470f491df25471cf51b0219cdf39f7f
push id36931
push useropoprus@mozilla.com
push dateThu, 19 Dec 2019 09:50:06 +0000
treeherdermozilla-central@5e8b48c8cd93 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjimb, jandem
bugs1602699
milestone73.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1602699 - Part 2: Change DebugAPI::onDebuggerStatement force-return to work via an error. r=jimb,jandem Differential Revision: https://phabricator.services.mozilla.com/D57282
js/src/debugger/DebugAPI-inl.h
js/src/debugger/DebugAPI.h
js/src/debugger/Debugger.cpp
js/src/jit/BaselineCodeGen.cpp
js/src/jit/JitFrames.cpp
js/src/jit/VMFunctions.cpp
js/src/jit/VMFunctions.h
js/src/vm/Interpreter.cpp
--- a/js/src/debugger/DebugAPI-inl.h
+++ b/js/src/debugger/DebugAPI-inl.h
@@ -138,22 +138,22 @@ ResumeMode DebugAPI::onNativeCall(JSCont
                                   CallReason reason) {
   if (!cx->realm()->isDebuggee()) {
     return ResumeMode::Continue;
   }
   return slowPathOnNativeCall(cx, args, reason);
 }
 
 /* static */
-ResumeMode DebugAPI::onDebuggerStatement(JSContext* cx,
-                                         AbstractFramePtr frame) {
-  if (!cx->realm()->isDebuggee()) {
-    return ResumeMode::Continue;
+bool DebugAPI::onDebuggerStatement(JSContext* cx, AbstractFramePtr frame) {
+  if (MOZ_UNLIKELY(cx->realm()->isDebuggee())) {
+    return slowPathOnDebuggerStatement(cx, frame);
   }
-  return slowPathOnDebuggerStatement(cx, frame);
+
+  return true;
 }
 
 /* static */
 ResumeMode DebugAPI::onExceptionUnwind(JSContext* cx, AbstractFramePtr frame) {
   if (!cx->realm()->isDebuggee()) {
     return ResumeMode::Continue;
   }
   return slowPathOnExceptionUnwind(cx, frame);
--- a/js/src/debugger/DebugAPI.h
+++ b/js/src/debugger/DebugAPI.h
@@ -229,18 +229,18 @@ class DebugAPI {
   /*
    * Announce to the debugger a |debugger;| statement on has been
    * encountered on the youngest JS frame on |cx|. Call whatever hooks have
    * been registered to observe this.
    *
    * Note that this method is called for all |debugger;| statements,
    * regardless of the frame's debuggee-ness.
    */
-  static inline ResumeMode onDebuggerStatement(JSContext* cx,
-                                               AbstractFramePtr frame);
+  static inline MOZ_MUST_USE bool onDebuggerStatement(JSContext* cx,
+                                                      AbstractFramePtr frame);
 
   /*
    * Announce to the debugger that an exception has been thrown and propagated
    * to |frame|. Call whatever hooks have been registered to observe this.
    */
   static inline ResumeMode onExceptionUnwind(JSContext* cx,
                                              AbstractFramePtr frame);
 
@@ -379,18 +379,18 @@ class DebugAPI {
       Handle<AbstractGeneratorObject*> genObj);
   static MOZ_MUST_USE bool slowPathCheckNoExecute(JSContext* cx,
                                                   HandleScript script);
   static ResumeMode slowPathOnEnterFrame(JSContext* cx, AbstractFramePtr frame);
   static ResumeMode slowPathOnResumeFrame(JSContext* cx,
                                           AbstractFramePtr frame);
   static ResumeMode slowPathOnNativeCall(JSContext* cx, const CallArgs& args,
                                          CallReason reason);
-  static ResumeMode slowPathOnDebuggerStatement(JSContext* cx,
-                                                AbstractFramePtr frame);
+  static MOZ_MUST_USE bool slowPathOnDebuggerStatement(JSContext* cx,
+                                                       AbstractFramePtr frame);
   static ResumeMode slowPathOnExceptionUnwind(JSContext* cx,
                                               AbstractFramePtr frame);
   static void slowPathOnNewWasmInstance(
       JSContext* cx, Handle<WasmInstanceObject*> wasmInstance);
   static void slowPathOnNewPromise(JSContext* cx,
                                    Handle<PromiseObject*> promise);
   static void slowPathOnPromiseSettled(JSContext* cx,
                                        Handle<PromiseObject*> promise);
--- a/js/src/debugger/Debugger.cpp
+++ b/js/src/debugger/Debugger.cpp
@@ -1034,46 +1034,47 @@ bool DebugAPI::slowPathOnNewGenerator(JS
         ok = false;
       }
     }
   });
   return ok;
 }
 
 /* static */
-ResumeMode DebugAPI::slowPathOnDebuggerStatement(JSContext* cx,
-                                                 AbstractFramePtr frame) {
+bool DebugAPI::slowPathOnDebuggerStatement(JSContext* cx,
+                                           AbstractFramePtr frame) {
   RootedValue rval(cx);
   ResumeMode resumeMode = Debugger::dispatchHook(
       cx,
       [](Debugger* dbg) -> bool {
         return dbg->getHook(Debugger::OnDebuggerStatement);
       },
       [&](Debugger* dbg) -> ResumeMode {
         return dbg->fireDebuggerStatement(cx, &rval);
       });
 
   switch (resumeMode) {
     case ResumeMode::Continue:
+      break;
     case ResumeMode::Terminate:
-      break;
+      return false;
 
     case ResumeMode::Return:
-      frame.setReturnValue(rval);
-      break;
+      DebugAPI::propagateForcedReturn(cx, frame, rval);
+      return false;
 
     case ResumeMode::Throw:
       cx->setPendingExceptionAndCaptureStack(rval);
-      break;
+      return false;
 
     default:
       MOZ_CRASH("Invalid onDebuggerStatement resume mode");
   }
 
-  return resumeMode;
+  return true;
 }
 
 /* static */
 ResumeMode DebugAPI::slowPathOnExceptionUnwind(JSContext* cx,
                                                AbstractFramePtr frame) {
   // Invoking more JS on an over-recursed stack or after OOM is only going
   // to result in more of the same error.
   if (cx->isThrowingOverRecursed() || cx->isThrowingOutOfMemory()) {
@@ -6493,25 +6494,24 @@ void DebugAPI::handleUnrecoverableIonBai
   // honor any further Debugger hooks on the frame, and need to ensure that
   // its Debugger.Frame entry is cleaned up.
   Debugger::removeFromFrameMapsAndClearBreakpointsIn(cx, frame);
 }
 
 /* static */
 void DebugAPI::propagateForcedReturn(JSContext* cx, AbstractFramePtr frame,
                                      HandleValue rval) {
-  // Invoking the interrupt handler is considered a step and invokes the
-  // youngest frame's onStep handler, if any. However, we cannot handle
-  // { return: ... } resumption values straightforwardly from the interrupt
-  // handler. Instead, we set the intended return value in the frame's rval
-  // slot and set the propagating-forced-return flag on the JSContext.
-  //
-  // The interrupt handler then returns false with no exception set,
-  // signaling an uncatchable exception. In the exception handlers, we then
-  // check for the special propagating-forced-return flag.
+  // The Debugger's hooks may return a value that affects the completion
+  // value of the given frame. For example, a hook may return `{ return: 42 }`
+  // to terminate the frame and return `42` as the final frame result.
+  // To accomplish this, the debugger treats these return values as if
+  // execution of the JS function has been terminated without a pending
+  // exception, but with a special flag. When the error is handled by the
+  // interpreter or JIT, the special flag and the error state will be cleared
+  // and execution will continue from the end of the frame.
   MOZ_ASSERT(!cx->isExceptionPending());
   cx->setPropagatingForcedReturn();
   frame.setReturnValue(rval);
 }
 
 /*** JS::dbg::Builder *******************************************************/
 
 Builder::Builder(JSContext* cx, js::Debugger* debugger)
--- a/js/src/jit/BaselineCodeGen.cpp
+++ b/js/src/jit/BaselineCodeGen.cpp
@@ -5134,35 +5134,26 @@ bool BaselineCodeGen<Handler>::emit_JSOP
 
   frame.push(R0);
   return true;
 }
 
 template <typename Handler>
 bool BaselineCodeGen<Handler>::emit_JSOP_DEBUGGER() {
   prepareVMCall();
-  pushBytecodePCArg();
 
   frame.assertSyncedStack();
   masm.loadBaselineFramePtr(BaselineFrameReg, R0.scratchReg());
   pushArg(R0.scratchReg());
 
-  using Fn = bool (*)(JSContext*, BaselineFrame*, jsbytecode*, bool*);
+  using Fn = bool (*)(JSContext*, BaselineFrame*);
   if (!callVM<Fn, jit::OnDebuggerStatement>()) {
     return false;
   }
 
-  // If the stub returns |true|, return the frame's return value.
-  Label done;
-  masm.branchTest32(Assembler::Zero, ReturnReg, ReturnReg, &done);
-  {
-    masm.loadValue(frame.addressOfReturnValue(), JSReturnOperand);
-    masm.jump(&returnNoDebugEpilogue_);
-  }
-  masm.bind(&done);
   return true;
 }
 
 template <typename Handler>
 bool BaselineCodeGen<Handler>::emitDebugEpilogue() {
   auto ifDebuggee = [this]() {
     // Move return value into the frame's rval slot.
     masm.storeValue(JSReturnOperand, frame.addressOfReturnValue());
--- a/js/src/jit/JitFrames.cpp
+++ b/js/src/jit/JitFrames.cpp
@@ -469,18 +469,17 @@ static void HandleExceptionBaseline(JSCo
     PCCounts* counts = script->getThrowCounts(pc);
     // If we failed to allocate, then skip the increment and continue to
     // handle the exception.
     if (counts) {
       counts->numExec()++;
     }
   }
 
-  // We may be propagating a forced return from the interrupt
-  // callback, which cannot easily force a return.
+  // We may be propagating a forced return from a debugger hook function.
   if (cx->isPropagatingForcedReturn()) {
     cx->clearPropagatingForcedReturn();
     ForcedReturn(cx, frame, pc, rfe);
     return;
   }
 
   bool hasTryNotes = !script->trynotes().empty();
 
--- a/js/src/jit/VMFunctions.cpp
+++ b/js/src/jit/VMFunctions.cpp
@@ -1177,35 +1177,18 @@ bool HandleDebugTrap(JSContext* cx, Base
 
     default:
       MOZ_CRASH("Invalid step/breakpoint resume mode");
   }
 
   return true;
 }
 
-bool OnDebuggerStatement(JSContext* cx, BaselineFrame* frame, jsbytecode* pc,
-                         bool* mustReturn) {
-  *mustReturn = false;
-
-  switch (DebugAPI::onDebuggerStatement(cx, frame)) {
-    case ResumeMode::Continue:
-      return true;
-
-    case ResumeMode::Return:
-      *mustReturn = true;
-      return jit::DebugEpilogue(cx, frame, pc, true);
-
-    case ResumeMode::Throw:
-    case ResumeMode::Terminate:
-      return false;
-
-    default:
-      MOZ_CRASH("Invalid OnDebuggerStatement resume mode");
-  }
+bool OnDebuggerStatement(JSContext* cx, BaselineFrame* frame) {
+  return DebugAPI::onDebuggerStatement(cx, frame);
 }
 
 bool GlobalHasLiveOnDebuggerStatement(JSContext* cx) {
   AutoUnsafeCallWithABI unsafe;
   return cx->realm()->isDebuggee() &&
          DebugAPI::hasDebuggerStatementHook(cx->global());
 }
 
--- a/js/src/jit/VMFunctions.h
+++ b/js/src/jit/VMFunctions.h
@@ -986,18 +986,17 @@ MOZ_MUST_USE bool NewArgumentsObject(JSC
 JSObject* CopyLexicalEnvironmentObject(JSContext* cx, HandleObject env,
                                        bool copySlots);
 
 JSObject* InitRestParameter(JSContext* cx, uint32_t length, Value* rest,
                             HandleObject templateObj, HandleObject res);
 
 MOZ_MUST_USE bool HandleDebugTrap(JSContext* cx, BaselineFrame* frame,
                                   uint8_t* retAddr, bool* mustReturn);
-MOZ_MUST_USE bool OnDebuggerStatement(JSContext* cx, BaselineFrame* frame,
-                                      jsbytecode* pc, bool* mustReturn);
+MOZ_MUST_USE bool OnDebuggerStatement(JSContext* cx, BaselineFrame* frame);
 MOZ_MUST_USE bool GlobalHasLiveOnDebuggerStatement(JSContext* cx);
 
 MOZ_MUST_USE bool EnterWith(JSContext* cx, BaselineFrame* frame,
                             HandleValue val, Handle<WithScope*> templ);
 MOZ_MUST_USE bool LeaveWith(JSContext* cx, BaselineFrame* frame);
 
 MOZ_MUST_USE bool PushLexicalEnv(JSContext* cx, BaselineFrame* frame,
                                  Handle<LexicalScope*> scope);
--- a/js/src/vm/Interpreter.cpp
+++ b/js/src/vm/Interpreter.cpp
@@ -1298,18 +1298,17 @@ again:
         MOZ_ASSERT_IF(regs.fp()->script()->hasScriptCounts(),
                       regs.fp()->script()->maybeGetPCCounts(regs.pc));
         return res;
     }
 
     ok = HandleClosingGeneratorReturn(cx, regs.fp(), ok);
     ok = DebugAPI::onLeaveFrame(cx, regs.fp(), regs.pc, ok);
   } else {
-    // We may be propagating a forced return from the interrupt
-    // callback, which cannot easily force a return.
+    // We may be propagating a forced return from a debugger hook function.
     if (MOZ_UNLIKELY(cx->isPropagatingForcedReturn())) {
       cx->clearPropagatingForcedReturn();
       if (!ForcedReturn(cx, regs)) {
         return ErrorReturnContinuation;
       }
       return SuccessfulReturnContinuation;
     }
 
@@ -3956,30 +3955,18 @@ static MOZ_NEVER_INLINE JS_HAZ_JSNATIVE_
         goto error;
       }
       REGS.sp--;
       REGS.sp[-1].setBoolean(cond);
     }
     END_CASE(JSOP_INSTANCEOF)
 
     CASE(JSOP_DEBUGGER) {
-      RootedValue rval(cx);
-      switch (DebugAPI::onDebuggerStatement(cx, REGS.fp())) {
-        case ResumeMode::Terminate:
-          goto error;
-        case ResumeMode::Continue:
-          break;
-        case ResumeMode::Return:
-          if (!ForcedReturn(cx, REGS)) {
-            goto error;
-          }
-          goto successful_return_continuation;
-        case ResumeMode::Throw:
-          goto error;
-        default:;
+      if (!DebugAPI::onDebuggerStatement(cx, REGS.fp())) {
+        goto error;
       }
     }
     END_CASE(JSOP_DEBUGGER)
 
     CASE(JSOP_PUSHLEXICALENV) {
       ReservedRooted<Scope*> scope(&rootScope0, script->getScope(REGS.pc));
 
       // Create block environment and push on scope chain.