Bug 1577639 - Do not attempt to clean up frames for non-OOM conditions. r=arai! draft
authorLogan Smyth <loganfsmyth@gmail.com>
Tue, 23 Jun 2020 15:32:24 -0700
changeset 3000033 8af515bc6fc45931324236db0df06a25a3bec922
parent 2995438 fa015682f65310416d2a40a146a7e8d004dd81d7
child 3000034 0231f07492c224006c41d878edf00466811153e4
push id558605
push userloganfsmyth@gmail.com
push dateWed, 24 Jun 2020 00:03:09 +0000
treeherdertry@205d063a3098 [default view] [failures only]
reviewersarai
bugs1577639
milestone79.0a1
Bug 1577639 - Do not attempt to clean up frames for non-OOM conditions. r=arai! The core issue is that we run removeFromFrameMapsAndClearBreakpointsIn(cx, from) even for non-OOM paths in this function, which is undesirable because that will remove the breakpoints for evaled scripts. In the non-OOM case there are no Debugger.Frame objects left in the frames map for the "frame" list, so there is no reason to actually call it in that case. This patch merges the two OOM handlers together for simplicity, though it is not technically necessary, it seems like the very minor increase in code run if there is an OOM is worth it to keep things centralized. Differential Revision: https://phabricator.services.mozilla.com/D80762
js/src/debugger/Debugger.cpp
--- a/js/src/debugger/Debugger.cpp
+++ b/js/src/debugger/Debugger.cpp
@@ -6247,93 +6247,85 @@ bool Debugger::observesWasm(wasm::Instan
     return false;
   }
   return observesGlobal(&instance->object()->global());
 }
 
 /* static */
 bool Debugger::replaceFrameGuts(JSContext* cx, AbstractFramePtr from,
                                 AbstractFramePtr to, ScriptFrameIter& iter) {
-  auto removeFromDebuggerFramesOnExit = MakeScopeExit([&] {
-    // Remove any remaining old entries on exit, as the 'from' frame will
-    // be gone. This is only done in the failure case. On failure, the
-    // removeToDebuggerFramesOnExit lambda below will rollback any frames
-    // that were replaced, resulting in !frameMaps(to). On success, the
-    // range will be empty, as all from Frame.Debugger instances will have
-    // been removed.
-    MOZ_ASSERT_IF(DebugAPI::inFrameMaps(to), !DebugAPI::inFrameMaps(from));
+  // Rekey missingScopes to maintain Debugger.Environment identity and
+  // forward liveScopes to point to the new frame.
+  DebugEnvironments::forwardLiveFrame(cx, from, to);
+
+  // If we hit an OOM anywhere in here, we need to make sure there aren't any
+  // Debugger.Frame objects left partially-initialized.
+  auto removeDebuggerFramesOnExit = MakeScopeExit([&] {
     removeFromFrameMapsAndClearBreakpointsIn(cx, from);
-
-    // Rekey missingScopes to maintain Debugger.Environment identity and
-    // forward liveScopes to point to the new frame.
-    DebugEnvironments::forwardLiveFrame(cx, from, to);
+    removeFromFrameMapsAndClearBreakpointsIn(cx, to);
+
+    MOZ_ASSERT(!DebugAPI::inFrameMaps(from));
+    MOZ_ASSERT(!DebugAPI::inFrameMaps(to));
   });
 
   // Forward live Debugger.Frame objects.
   Rooted<DebuggerFrameVector> frames(cx, DebuggerFrameVector(cx));
   if (!getDebuggerFrames(from, &frames)) {
     // An OOM here means that all Debuggers' frame maps still contain
     // entries for 'from' and no entries for 'to'. Since the 'from' frame
-    // will be gone, they are removed by removeFromDebuggerFramesOnExit
+    // will be gone, they are removed by removeDebuggerFramesOnExit
     // above.
     return false;
   }
 
-  // If during the loop below we hit an OOM, we must also rollback any of
-  // the frames that were successfully replaced. For OSR frames, OOM here
-  // means those frames will pop from the OSR trampoline, which does not
-  // call Debugger::onLeaveFrame.
-  auto removeToDebuggerFramesOnExit =
-      MakeScopeExit([&] { removeFromFrameMapsAndClearBreakpointsIn(cx, to); });
-
   for (size_t i = 0; i < frames.length(); i++) {
     HandleDebuggerFrame frameobj = frames[i];
     Debugger* dbg = Debugger::fromChildJSObject(frameobj);
 
     // Update frame object's ScriptFrameIter::data pointer.
     frameobj->freeFrameIterData(cx->runtime()->defaultFreeOp());
     ScriptFrameIter::Data* data = iter.copyData();
     if (!data) {
       // An OOM here means that some Debuggers' frame maps may still
       // contain entries for 'from' and some Debuggers' frame maps may
-      // also contain entries for 'to'. Thus both
-      // removeFromDebuggerFramesOnExit and
-      // removeToDebuggerFramesOnExit must both run.
+      // also contain entries for 'to'. Thus removeDebuggerFramesOnExit
+      // must run.
       //
       // The current frameobj in question is still in its Debugger's
       // frame map keyed by 'from', so it will be covered by
-      // removeFromDebuggerFramesOnExit.
+      // removeDebuggerFramesOnExit.
       return false;
     }
     frameobj->setFrameIterData(data);
 
     // Remove old frame.
     dbg->frames.remove(from);
 
     // Add the frame object with |to| as key.
     if (!dbg->frames.putNew(to, frameobj)) {
-      // This OOM is subtle. At this point, both
-      // removeFromDebuggerFramesOnExit and removeToDebuggerFramesOnExit
-      // must both run for the same reason given above.
+      // This OOM is subtle. At this point, removeDebuggerFramesOnExit
+      // must run for the same reason given above.
       //
       // The difference is that the current frameobj is no longer in its
       // Debugger's frame map, so it will not be cleaned up by neither
       // lambda. Manually clean it up here.
       JSFreeOp* fop = cx->runtime()->defaultFreeOp();
       frameobj->freeFrameIterData(fop);
       frameobj->maybeDecrementStepperCounter(fop, to);
 
       ReportOutOfMemory(cx);
       return false;
     }
   }
 
   // All frames successfuly replaced, cancel the rollback.
-  removeToDebuggerFramesOnExit.release();
-
+  removeDebuggerFramesOnExit.release();
+
+  MOZ_ASSERT(!DebugAPI::inFrameMaps(from));
+  MOZ_ASSERT_IF(!frames.empty(), DebugAPI::inFrameMaps(to));
   return true;
 }
 
 /* static */
 bool DebugAPI::inFrameMaps(AbstractFramePtr frame) {
   bool foundAny = false;
   Debugger::forEachDebuggerFrame(
       frame, [&](DebuggerFrame* frameobj) { foundAny = true; });