Bug 1647342 - Part 5: Move management of the generatorFrames collection. r=arai! draft
authorLogan Smyth <loganfsmyth@gmail.com>
Tue, 23 Jun 2020 23:58:37 +0000
changeset 3000004 0edefdaa100d10977af2ea69566c3d1ef1931947
parent 3000003 76cad3b6b8f87c8948d8d355803e8d04e44d007f
child 3000005 7491eefcecfddc2ed39bb986b306933d3cfb6354
push id558601
push userreviewbot
push dateTue, 23 Jun 2020 23:59:03 +0000
treeherdertry@7491eefcecfd [default view] [failures only]
reviewersarai
bugs1647342
milestone79.0a1
Bug 1647342 - Part 5: Move management of the generatorFrames collection. r=arai! Summary: It is difficult to follow the overall lifecycle of entries in the generatorFrames list when it is mutated in many places across many files. This patch's primary aim is to begin centralizing the handling of this map a little bit by keeping it all inside Debugger.cpp so that the DebuggerFrame instance itself does not actually care whether it is found in that map. Differential Revision: https://phabricator.services.mozilla.com/D80738 Depends on D80737 Test Plan: Reviewers: Subscribers: Bug #: 1647342 Differential Diff: PHID-DIFF-ukfthn5qshjnhhhlieli
js/src/debugger/Debugger.cpp
js/src/debugger/Frame.cpp
js/src/debugger/Frame.h
--- a/js/src/debugger/Debugger.cpp
+++ b/js/src/debugger/Debugger.cpp
@@ -634,75 +634,102 @@ bool Debugger::getFrame(JSContext* cx, c
       genObj = GetGeneratorObjectForFrame(cx, referent);
 
       // If this frame has a generator associated with it, but no on-stack
       // Debugger.Frame object was found, there should not be a suspended
       // Debugger.Frame either because otherwise slowPathOnResumeFrame would
       // have already populated the "frames" map with a Debugger.Frame.
       MOZ_ASSERT_IF(genObj, !generatorFrames.has(genObj));
 
+      // If the frame's generator is closed, there is no way to associate the
+      // generator with the frame successfully because there is no way to
+      // get the generator's callee script, and even if we could, having it
+      // there would in no way affect the behavior of the frame.
+      if (genObj && genObj->isClosed()) {
+        genObj = nullptr;
+      }
+
       // If no AbstractGeneratorObject exists yet, we create a Debugger.Frame
       // below anyway, and Debugger::onNewGenerator() will associate it
       // with the AbstractGeneratorObject later when we hit JSOp::Generator.
     }
 
     // Create and populate the Debugger.Frame object.
     RootedObject proto(
         cx, &object->getReservedSlot(JSSLOT_DEBUG_FRAME_PROTO).toObject());
     RootedNativeObject debugger(cx, object);
 
     RootedDebuggerFrame frame(
         cx, DebuggerFrame::create(cx, proto, debugger, &iter, genObj));
     if (!frame) {
       return false;
     }
 
+    auto terminateDebuggerFrameGuard = MakeScopeExit([&] {
+      frame->freeFrameIterData(cx->defaultFreeOp());
+      frame->clearGeneratorInfo(cx->runtime()->defaultFreeOp());
+      generatorFrames.remove(genObj);
+    });
+
+    if (genObj) {
+      DependentAddPtr<GeneratorWeakMap> genPtr(cx, generatorFrames, genObj);
+      if (!genPtr.add(cx, generatorFrames, genObj, frame)) {
+        return false;
+      }
+    }
+
     if (!ensureExecutionObservabilityOfFrame(cx, referent)) {
       return false;
     }
 
     if (!frames.add(p, referent, frame)) {
-      frame->freeFrameIterData(cx->defaultFreeOp());
-      frame->clearGeneratorInfo(cx->runtime()->defaultFreeOp(), this);
       ReportOutOfMemory(cx);
       return false;
     }
+
+    terminateDebuggerFrameGuard.release();
   }
 
   result.set(p->value());
   return true;
 }
 
 bool Debugger::getFrame(JSContext* cx, Handle<AbstractGeneratorObject*> genObj,
                         MutableHandleDebuggerFrame result) {
   // To create a Debugger.Frame for a running generator, we'd also need a
   // FrameIter for its stack frame. We could make this work by searching the
   // stack for the generator's frame, but for the moment, we only need this
   // function to handle generators we've found on promises' reaction records,
   // which should always be suspended.
-  MOZ_ASSERT(!genObj->isRunning());
+  MOZ_ASSERT(genObj->isSuspended());
 
   // Do we have an existing Debugger.Frame for this generator?
-  GeneratorWeakMap::Ptr gp = generatorFrames.lookup(genObj);
-  if (gp) {
-    MOZ_ASSERT(&gp->value()->unwrappedGenerator() == genObj);
-    result.set(gp->value());
+  DependentAddPtr<GeneratorWeakMap> p(cx, generatorFrames, genObj);
+  if (p) {
+    MOZ_ASSERT(&p->value()->unwrappedGenerator() == genObj);
+    result.set(p->value());
     return true;
   }
 
   // Create a new Debugger.Frame.
   RootedObject proto(
       cx, &object->getReservedSlot(JSSLOT_DEBUG_FRAME_PROTO).toObject());
   RootedNativeObject debugger(cx, object);
 
   result.set(DebuggerFrame::create(cx, proto, debugger, nullptr, genObj));
   if (!result) {
     return false;
   }
 
+  if (!p.add(cx, generatorFrames, genObj, result)) {
+    result->freeFrameIterData(cx->defaultFreeOp());
+    result->clearGeneratorInfo(cx->runtime()->defaultFreeOp());
+    return false;
+  }
+
   return true;
 }
 
 static bool DebuggerExists(
     GlobalObject* global, const std::function<bool(Debugger* dbg)>& predicate) {
   // The GC analysis can't determine that the predicate can't GC, so let it know
   // explicitly.
   JS::AutoSuppressGCAnalysis nogc;
@@ -1171,33 +1198,39 @@ bool DebugAPI::slowPathOnNewGenerator(JS
                                       Handle<AbstractGeneratorObject*> genObj) {
   // This is called from JSOp::Generator, after default parameter expressions
   // are evaluated and well after onEnterFrame, so Debugger.Frame objects for
   // `frame` may already have been exposed to debugger code. The
   // AbstractGeneratorObject for this generator call, though, has just been
   // created. It must be associated with any existing Debugger.Frames.
   bool ok = true;
   Debugger::forEachDebuggerFrame(
-      frame, [&](Debugger*, DebuggerFrame* frameObjPtr) {
+      frame, [&](Debugger* dbg, DebuggerFrame* frameObjPtr) {
         if (!ok) {
           return;
         }
 
         RootedDebuggerFrame frameObj(cx, frameObjPtr);
-        {
-          AutoRealm ar(cx, frameObj);
-
-          if (!frameObj->setGeneratorInfo(cx, genObj)) {
-            ReportOutOfMemory(cx);
-
-            // This leaves `genObj` and `frameObj` unassociated. It's OK
-            // because we won't pause again with this generator on the stack:
-            // the caller will immediately discard `genObj` and unwind `frame`.
-            ok = false;
-          }
+
+        AutoRealm ar(cx, frameObj);
+
+        if (!frameObj->setGeneratorInfo(cx, genObj)) {
+          ReportOutOfMemory(cx);
+
+          // This leaves `genObj` and `frameObj` unassociated. It's OK
+          // because we won't pause again with this generator on the stack:
+          // the caller will immediately discard `genObj` and unwind `frame`.
+          ok = false;
+          return;
+        }
+
+        DependentAddPtr<Debugger::GeneratorWeakMap> genPtr(
+            cx, dbg->generatorFrames, genObj);
+        if (!genPtr.add(cx, dbg->generatorFrames, genObj, frameObj)) {
+          ok = false;
         }
       });
   return ok;
 }
 
 /* static */
 bool DebugAPI::slowPathOnDebuggerStatement(JSContext* cx,
                                            AbstractFramePtr frame) {
@@ -3809,17 +3842,18 @@ void DebugAPI::sweepAll(JSFreeOp* fop) {
     //
     // Since DebugAPI::sweepAll is called after everything is marked, but before
     // anything has been finalized, this is the perfect place to drop the count.
     if (dbg->zone()->isGCSweeping()) {
       for (Debugger::GeneratorWeakMap::Enum e(dbg->generatorFrames); !e.empty();
            e.popFront()) {
         DebuggerFrame* frameObj = e.front().value();
         if (IsAboutToBeFinalizedUnbarriered(&frameObj)) {
-          frameObj->clearGeneratorInfo(fop, dbg, &e);
+          e.removeFront();
+          frameObj->clearGeneratorInfo(fop);
         }
       }
     }
 
     // Detach dying debuggers and debuggees from each other. Since this
     // requires access to both objects it must be done before either
     // object is finalized.
     bool debuggerDying = IsAboutToBeFinalized(&dbg->object);
@@ -4697,17 +4731,18 @@ void Debugger::removeDebuggeeGlobal(JSFr
   // is going away, it doesn't care about the state of its generatorFrames
   // table, and the Debugger.Frame finalizer will fix up the generator observer
   // counts.
   if (fromSweep == FromSweep::No) {
     for (GeneratorWeakMap::Enum e(generatorFrames); !e.empty(); e.popFront()) {
       AbstractGeneratorObject& genObj = *e.front().key();
       DebuggerFrame& frameObj = *e.front().value();
       if (genObj.isClosed() || &genObj.callee().global() == global) {
-        frameObj.clearGeneratorInfo(fop, this, &e);
+        e.removeFront();
+        frameObj.clearGeneratorInfo(fop);
       }
     }
   }
 
   auto& globalDebuggersVector = global->getDebuggers();
 
   // The relation must be removed from up to three places:
   // globalDebuggersVector and debuggees for sure, and possibly the
@@ -6308,17 +6343,18 @@ void Debugger::removeFromFrameMapsAndCle
       if (!suspending) {
         // Terminally exiting a generator.
 #if DEBUG
         AbstractGeneratorObject& genObj = frameobj->unwrappedGenerator();
         GeneratorWeakMap::Ptr p = dbg->generatorFrames.lookup(&genObj);
         MOZ_ASSERT(p);
         MOZ_ASSERT(p->value() == frameobj);
 #endif
-        frameobj->clearGeneratorInfo(fop, dbg);
+        dbg->generatorFrames.remove(&frameobj->unwrappedGenerator());
+        frameobj->clearGeneratorInfo(fop);
       }
     } else {
       frameobj->maybeDecrementStepperCounter(fop, frame);
     }
   });
 
   // If this is an eval frame, then from the debugger's perspective the
   // script is about to be destroyed. Remove any breakpoints in it.
--- a/js/src/debugger/Frame.cpp
+++ b/js/src/debugger/Frame.cpp
@@ -347,74 +347,47 @@ JSScript* js::DebuggerFrame::generatorSc
   return generatorInfo()->generatorScript();
 }
 #endif
 
 bool DebuggerFrame::setGeneratorInfo(JSContext* cx,
                                      Handle<AbstractGeneratorObject*> genObj) {
   cx->check(this);
 
-  Debugger::GeneratorWeakMap::AddPtr p =
-      owner()->generatorFrames.lookupForAdd(genObj);
-  if (p) {
-    MOZ_ASSERT(p->value() == this);
-    MOZ_ASSERT(&unwrappedGenerator() == genObj);
-    return true;
-  }
+  MOZ_ASSERT(!hasGeneratorInfo());
+  MOZ_ASSERT(!genObj->isClosed());
 
-  // There are three relations we must establish:
+  // There are two relations we must establish:
   //
   // 1) The DebuggerFrame must point to the AbstractGeneratorObject.
   //
-  // 2) generatorFrames must map the AbstractGeneratorObject to the
-  //    DebuggerFrame.
-  //
-  // 3) The generator's script's observer count must be bumped.
-  UniquePtr<GeneratorInfo> info;
-  RootedScript script(cx);
-  if (!genObj->isClosed()) {
-    script = genObj->callee().nonLazyScript();
-    info.reset(cx->new_<GeneratorInfo>(genObj, script));
-    if (!info) {
-      ReportOutOfMemory(cx);
-      return false;
-    }
-  }
+  // 2) The generator's script's observer count must be bumped.
 
-  if (!owner()->generatorFrames.relookupOrAdd(p, genObj, this)) {
-    ReportOutOfMemory(cx);
+  RootedScript script(cx, genObj->callee().nonLazyScript());
+  auto info = cx->make_unique<GeneratorInfo>(genObj, script);
+  if (!info) {
     return false;
   }
-  auto generatorFramesGuard =
-      MakeScopeExit([&] { owner()->generatorFrames.remove(genObj); });
 
-  if (script) {
-    AutoRealm ar(cx, script);
+  AutoRealm ar(cx, script);
 
-    // All frames running a debuggee script must themselves be marked as
-    // debuggee frames. Bumping a script's generator observer count makes it a
-    // debuggee, so we need to mark all frames on the stack running it as
-    // debuggees as well, not just this one. This call takes care of all that.
-    if (!Debugger::ensureExecutionObservabilityOfScript(cx, script)) {
-      return false;
-    }
-
-    if (!DebugScript::incrementGeneratorObserverCount(cx, script)) {
-      return false;
-    }
+  // All frames running a debuggee script must themselves be marked as
+  // debuggee frames. Bumping a script's generator observer count makes it a
+  // debuggee, so we need to mark all frames on the stack running it as
+  // debuggees as well, not just this one. This call takes care of all that.
+  if (!Debugger::ensureExecutionObservabilityOfScript(cx, script)) {
+    return false;
   }
 
-  if (info) {
-    InitReservedSlot(this, GENERATOR_INFO_SLOT, info.release(),
-                     MemoryUse::DebuggerFrameGeneratorInfo);
-  } else {
-    setReservedSlot(GENERATOR_INFO_SLOT, UndefinedValue());
+  if (!DebugScript::incrementGeneratorObserverCount(cx, script)) {
+    return false;
   }
-  generatorFramesGuard.release();
 
+  InitReservedSlot(this, GENERATOR_INFO_SLOT, info.release(),
+                   MemoryUse::DebuggerFrameGeneratorInfo);
   return true;
 }
 
 void DebuggerFrame::clearGeneratorInfo(JSFreeOp* fop) {
   if (!hasGeneratorInfo()) {
     return;
   }
 
@@ -433,35 +406,16 @@ void DebuggerFrame::clearGeneratorInfo(J
     maybeDecrementStepperCounter(fop, generatorScript);
   }
 
   // 1) The DebuggerFrame must no longer point to the AbstractGeneratorObject.
   setReservedSlot(GENERATOR_INFO_SLOT, UndefinedValue());
   fop->delete_(this, info, MemoryUse::DebuggerFrameGeneratorInfo);
 }
 
-void DebuggerFrame::clearGeneratorInfo(
-    JSFreeOp* fop, Debugger* owner,
-    Debugger::GeneratorWeakMap::Enum* maybeGeneratorFramesEnum) {
-  if (!hasGeneratorInfo()) {
-    return;
-  }
-
-  // 2) generatorFrames must no longer map the AbstractGeneratorObject to the
-  // DebuggerFrame.
-  GeneratorInfo* info = generatorInfo();
-  if (maybeGeneratorFramesEnum) {
-    maybeGeneratorFramesEnum->removeFront();
-  } else {
-    owner->generatorFrames.remove(&info->unwrappedGenerator());
-  }
-
-  clearGeneratorInfo(fop);
-}
-
 /* static */
 bool DebuggerFrame::getCallee(JSContext* cx, HandleDebuggerFrame frame,
                               MutableHandleDebuggerObject result) {
   RootedObject callee(cx);
   if (frame->isOnStack()) {
     AbstractFramePtr referent = DebuggerFrame::getReferent(frame);
     if (referent.isFunctionFrame()) {
       callee = referent.callee();
--- a/js/src/debugger/Frame.h
+++ b/js/src/debugger/Frame.h
@@ -242,19 +242,16 @@ class DebuggerFrame : public NativeObjec
    * necessary.)
    *
    * If maybeGeneratorFramesEnum is non-null, use it to remove this frame's
    * entry from the Debugger's generatorFrames weak map. In this case, this
    * function will not otherwise disturb generatorFrames. Passing the enum
    * allows this function to be used while iterating over generatorFrames.
    */
   void clearGeneratorInfo(JSFreeOp* fop);
-  void clearGeneratorInfo(
-      JSFreeOp* fop, Debugger* owner,
-      Debugger::GeneratorWeakMap::Enum* maybeGeneratorFramesEnum = nullptr);
 
   /*
    * Called after a generator/async frame is resumed, before exposing this
    * Debugger.Frame object to any hooks.
    */
   bool resume(const FrameIter& iter);
 
   bool hasAnyHooks() const;