Bug 1647342 - Part 7: Remove unnecessary special-casing of OOM in replaceFrameGuts. r=arai! draft
authorLogan Smyth <loganfsmyth@gmail.com>
Thu, 18 Jun 2020 13:41:58 -0700
changeset 3000040 5743c6c64e0bb9aca126642d0937a1e144a2397c
parent 3000039 0376ee157f2eac9d1013e7558b9bd05132a7de7b
child 3000041 e60c99ee5b9aca765d95167f07d05a24d0d883b5
push id558605
push userloganfsmyth@gmail.com
push dateWed, 24 Jun 2020 00:03:09 +0000
treeherdertry@205d063a3098 [default view] [failures only]
reviewersarai
bugs1647342
milestone79.0a1
Bug 1647342 - Part 7: Remove unnecessary special-casing of OOM in replaceFrameGuts. r=arai! This patch waits to remove the frame from the "from" map until after it has been added to the "to" map, so that we can rely on the existing scope guard to clean up any frames if we get an OOM. Differential Revision: https://phabricator.services.mozilla.com/D80740
js/src/debugger/Debugger.cpp
--- a/js/src/debugger/Debugger.cpp
+++ b/js/src/debugger/Debugger.cpp
@@ -6248,16 +6248,18 @@ bool Debugger::observesWasm(wasm::Instan
     return false;
   }
   return observesGlobal(&instance->object()->global());
 }
 
 /* static */
 bool Debugger::replaceFrameGuts(JSContext* cx, AbstractFramePtr from,
                                 AbstractFramePtr to, ScriptFrameIter& iter) {
+  MOZ_ASSERT(from != to);
+
   // 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);
@@ -6281,34 +6283,25 @@ bool Debugger::replaceFrameGuts(JSContex
     HandleDebuggerFrame frameobj = frames[i];
     Debugger* dbg = Debugger::fromChildJSObject(frameobj);
 
     // Update frame object's ScriptFrameIter::data pointer.
     if (!frameobj->replaceFrameIterData(cx, iter)) {
       return false;
     }
 
-    // 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, 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;
     }
+
+    // Remove the old frame entry after all fallible operations are completed
+    // so that an OOM will be able to clean up properly.
+    dbg->frames.remove(from);
   }
 
   // All frames successfuly replaced, cancel the rollback.
   removeDebuggerFramesOnExit.release();
 
   MOZ_ASSERT(!DebugAPI::inFrameMaps(from));
   MOZ_ASSERT_IF(!frames.empty(), DebugAPI::inFrameMaps(to));
   return true;