Bug 1602699 - Part 4: Change DebugAPI::onTrap force-return to work via an error. r=jimb,jandem
authorLogan Smyth <loganfsmyth@gmail.com>
Thu, 19 Dec 2019 00:56:58 +0000
changeset 507677 4cdc7061129b8008991ff63a59869bce28cfb1ee
parent 507676 0340540c6470f491df25471cf51b0219cdf39f7f
child 507678 b2ad424d48824465317c34f09f7273339ea09207
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 4: Change DebugAPI::onTrap force-return to work via an error. r=jimb,jandem Differential Revision: https://phabricator.services.mozilla.com/D57284
js/src/debugger/DebugAPI.h
js/src/debugger/Debugger.cpp
js/src/jit/VMFunctions.cpp
js/src/vm/Interpreter.cpp
js/src/wasm/WasmBuiltins.cpp
--- a/js/src/debugger/DebugAPI.h
+++ b/js/src/debugger/DebugAPI.h
@@ -259,17 +259,17 @@ class DebugAPI {
    * throw, or vice versa: we can redirect to a complete copy of the
    * alternative path, containing its own call to onLeaveFrame.)
    */
   static inline MOZ_MUST_USE bool onLeaveFrame(JSContext* cx,
                                                AbstractFramePtr frame,
                                                jsbytecode* pc, bool ok);
 
   // Call any breakpoint handlers for the current scripted location.
-  static ResumeMode onTrap(JSContext* cx, MutableHandleValue vp);
+  static MOZ_MUST_USE bool onTrap(JSContext* cx);
 
   // Call any stepping handlers for the current scripted location.
   static MOZ_MUST_USE bool onSingleStep(JSContext* cx);
 
   // Notify any Debugger instances observing this promise's global that a new
   // promise was allocated.
   static inline void onNewPromise(JSContext* cx,
                                   Handle<PromiseObject*> promise);
--- a/js/src/debugger/Debugger.cpp
+++ b/js/src/debugger/Debugger.cpp
@@ -2387,17 +2387,17 @@ void DebugAPI::slowPathOnNewWasmInstance
     cx->clearPendingException();
     return;
   }
 
   MOZ_ASSERT(resumeMode == ResumeMode::Continue);
 }
 
 /* static */
-ResumeMode DebugAPI::onTrap(JSContext* cx, MutableHandleValue vp) {
+bool DebugAPI::onTrap(JSContext* cx) {
   FrameIter iter(cx);
   JS::AutoSaveExceptionState savedExc(cx);
   Rooted<GlobalObject*> global(cx);
   BreakpointSite* site;
   bool isJS;       // true when iter.hasScript(), false when iter.isWasm()
   jsbytecode* pc;  // valid when isJS == true
   uint32_t bytecodeOffset;  // valid when isJS == false
   if (iter.hasScript()) {
@@ -2420,27 +2420,27 @@ ResumeMode DebugAPI::onTrap(JSContext* c
   // Build list of breakpoint handlers.
   //
   // This does not need to be rooted: since the JSScript/WasmInstance is on the
   // stack, the Breakpoints will not be GC'd. However, they may be deleted, and
   // we check for that case below.
   Vector<Breakpoint*> triggered(cx);
   for (Breakpoint* bp = site->firstBreakpoint(); bp; bp = bp->nextInSite()) {
     if (!triggered.append(bp)) {
-      return ResumeMode::Terminate;
+      return false;
     }
   }
 
   if (triggered.length() > 0) {
     // Preserve the debuggee's microtask event queue while we run the hooks, so
     // the debugger's microtask checkpoints don't run from the debuggee's
     // microtasks, and vice versa.
     JS::AutoDebuggerJobQueueInterruption adjqi;
     if (!adjqi.init(cx)) {
-      return ResumeMode::Terminate;
+      return false;
     }
 
     for (Breakpoint* bp : triggered) {
       // Handlers can clear breakpoints. Check that bp still exists.
       if (!site || !site->hasBreakpoint(bp)) {
         continue;
       }
 
@@ -2457,59 +2457,61 @@ ResumeMode DebugAPI::onTrap(JSContext* c
       if (dbg->debuggees.has(global)) {
         Maybe<AutoRealm> ar;
         ar.emplace(cx, dbg->object);
         EnterDebuggeeNoExecute nx(cx, *dbg, adjqi);
 
         RootedValue scriptFrame(cx);
         if (!dbg->getFrame(cx, iter, &scriptFrame)) {
           dbg->reportUncaughtException(ar);
-          return ResumeMode::Terminate;
+          return false;
         }
 
         // Re-wrap the breakpoint's handler for the Debugger's compartment. When
         // the handler and the Debugger are in the same compartment (the usual
         // case), this actually unwraps it, but there's no requirement that they
         // be in the same compartment, so we can't be sure.
         Rooted<JSObject*> handler(cx, bp->handler);
         if (!cx->compartment()->wrap(cx, &handler)) {
           dbg->reportUncaughtException(ar);
-          return ResumeMode::Terminate;
+          return false;
         }
 
         RootedValue rv(cx);
+        RootedValue rval(cx);
         bool ok = CallMethodIfPresent(cx, handler, "hit", 1,
                                       scriptFrame.address(), &rv);
         ResumeMode resumeMode = dbg->processHandlerResult(
-            ar, ok, rv, iter.abstractFramePtr(), iter.pc(), vp);
+            ar, ok, rv, iter.abstractFramePtr(), iter.pc(), &rval);
         adjqi.runJobs();
 
         if (resumeMode != ResumeMode::Continue) {
           savedExc.drop();
-          return resumeMode;
+
+          if (resumeMode == ResumeMode::Return) {
+            DebugAPI::propagateForcedReturn(cx, iter.abstractFramePtr(), rval);
+          } else if (resumeMode == ResumeMode::Throw) {
+            cx->setPendingExceptionAndCaptureStack(rval);
+          } else {
+            MOZ_ASSERT(resumeMode == ResumeMode::Terminate);
+          }
+          return false;
         }
 
         // Calling JS code invalidates site. Reload it.
         if (isJS) {
           site = DebugScript::getBreakpointSite(iter.script(), pc);
         } else {
           site = iter.wasmInstance()->debug().getBreakpointSite(bytecodeOffset);
         }
       }
     }
   }
 
-  // By convention, return the true op to the interpreter in vp, and return
-  // undefined in vp to the wasm debug trap.
-  if (isJS) {
-    vp.setInt32(JSOp(*pc));
-  } else {
-    vp.set(UndefinedValue());
-  }
-  return ResumeMode::Continue;
+  return true;
 }
 
 /* static */
 bool DebugAPI::onSingleStep(JSContext* cx) {
   FrameIter iter(cx);
 
   // We may be stepping over a JSOP_EXCEPTION, that pushes the context's
   // pending exception for a 'catch' clause to handle. Don't let the onStep
--- a/js/src/jit/VMFunctions.cpp
+++ b/js/src/jit/VMFunctions.cpp
@@ -1146,41 +1146,18 @@ bool HandleDebugTrap(JSContext* cx, Base
   }
 
   MOZ_ASSERT(frame->isDebuggee());
 
   if (DebugAPI::stepModeEnabled(script) && !DebugAPI::onSingleStep(cx)) {
     return false;
   }
 
-  RootedValue rval(cx);
-  ResumeMode resumeMode = ResumeMode::Continue;
-
-  if (DebugAPI::hasBreakpointsAt(script, pc)) {
-    resumeMode = DebugAPI::onTrap(cx, &rval);
-  }
-
-  switch (resumeMode) {
-    case ResumeMode::Continue:
-      break;
-
-    case ResumeMode::Terminate:
-      return false;
-
-    case ResumeMode::Return:
-      *mustReturn = true;
-      frame->setReturnValue(rval);
-      return jit::DebugEpilogue(cx, frame, pc, true);
-
-    case ResumeMode::Throw:
-      cx->setPendingExceptionAndCaptureStack(rval);
-      return false;
-
-    default:
-      MOZ_CRASH("Invalid step/breakpoint resume mode");
+  if (DebugAPI::hasBreakpointsAt(script, pc) && !DebugAPI::onTrap(cx)) {
+    return false;
   }
 
   return true;
 }
 
 bool OnDebuggerStatement(JSContext* cx, BaselineFrame* frame) {
   return DebugAPI::onDebuggerStatement(cx, frame);
 }
--- a/js/src/vm/Interpreter.cpp
+++ b/js/src/vm/Interpreter.cpp
@@ -1918,35 +1918,19 @@ static MOZ_NEVER_INLINE JS_HAZ_JSNATIVE_
           moreInterrupts = true;
         }
 
         if (DebugAPI::hasAnyBreakpointsOrStepMode(script)) {
           moreInterrupts = true;
         }
 
         if (DebugAPI::hasBreakpointsAt(script, REGS.pc)) {
-          RootedValue rval(cx);
-          ResumeMode mode = DebugAPI::onTrap(cx, &rval);
-          switch (mode) {
-            case ResumeMode::Terminate:
-              goto error;
-            case ResumeMode::Return:
-              REGS.fp()->setReturnValue(rval);
-              if (!ForcedReturn(cx, REGS)) {
-                goto error;
-              }
-              goto successful_return_continuation;
-            case ResumeMode::Throw:
-              cx->setPendingExceptionAndCaptureStack(rval);
-              goto error;
-            default:
-              break;
+          if (!DebugAPI::onTrap(cx)) {
+            goto error;
           }
-          MOZ_ASSERT(mode == ResumeMode::Continue);
-          MOZ_ASSERT(rval.isInt32() && rval.toInt32() == op);
         }
       }
 
       MOZ_ASSERT(activation.opMask() == EnableInterruptsPseudoOpcode);
       if (!moreInterrupts) {
         activation.clearInterruptsMask();
       }
 
--- a/js/src/wasm/WasmBuiltins.cpp
+++ b/js/src/wasm/WasmBuiltins.cpp
@@ -340,25 +340,23 @@ static bool WasmHandleDebugTrap() {
         // TODO properly handle forced return.
         JS_ReportErrorASCII(cx,
                             "Unexpected resumption value from onSingleStep");
       }
       return false;
     }
   }
   if (debug.hasBreakpointSite(site->lineOrBytecode())) {
-    RootedValue result(cx, UndefinedValue());
-    ResumeMode mode = DebugAPI::onTrap(cx, &result);
-    if (mode == ResumeMode::Return) {
-      // TODO properly handle ResumeMode::Return.
-      JS_ReportErrorASCII(
-          cx, "Unexpected resumption value from breakpoint handler");
-      return false;
-    }
-    if (mode != ResumeMode::Continue) {
+    if (!DebugAPI::onTrap(cx)) {
+      if (cx->isPropagatingForcedReturn()) {
+        cx->clearPropagatingForcedReturn();
+        // TODO properly handle forced return.
+        JS_ReportErrorASCII(
+            cx, "Unexpected resumption value from breakpoint handler");
+      }
       return false;
     }
   }
   return true;
 }
 
 // Unwind the entire activation in response to a thrown exception. This function
 // is responsible for notifying the debugger of each unwound frame. The return