Bug 1647342 - Part 5: Move management of the generatorFrames collection. r=arai! draft
authorLogan Smyth <loganfsmyth@gmail.com>
Wed, 24 Jun 2020 00:00:40 +0000
changeset 3000019 789c1c044c58913e126309da8338e244213a4a82
parent 3000018 7a9369fb158e593b3aaf1097c89ee54c6dadd7db
child 3000020 992c71c04e554092d22b3c6ab28c0dc33fc1bbde
push id558603
push userreviewbot
push dateWed, 24 Jun 2020 00:01:15 +0000
treeherdertry@c09587b1a222 [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;