Bug 1560754: Remove DebuggeeFrameGeneratorScript. r=jorendorff

Stop inserting DebuggeeFrameGeneratorScript keys in the cross-compartment
wrapper table for the edges from Debugger.Frames for generator / async calls to
the generators' scripts. The wrappers are unnecessary, and since they're not
unique when multiple Debugger.Frames refer to different calls of the same
generator, we can't easily tell when to remove them.

Differential Revision: https://phabricator.services.mozilla.com/D35617

--HG--
extra : moz-landing-system : lando

js/src/jit-test/tests/debug/bug1557343-2.js

@@ -0,0 +1,30 @@
+// |jit-test| --no-ggc
+// Don't crash when two Debugger.Frames refer to the same generator script, and
+// then one returns.
+
+var g = newGlobal({ newCompartment: true });
+g.eval(`
+  function* gen() {
+    debugger;
+    yield 1;
+  }
+
+  function use_gen() {
+    var gen1 = gen();
+    var gen2 = gen();
+
+    gen1.next();
+    gen2.next();
+
+    gen1.next();
+    gen2.next();
+  }
+`);
+
+var dbg = new Debugger(g);
+var frame;
+dbg.onDebuggerStatement = f => {
+  frame = f;
+};
+
+g.use_gen();

js/src/vm/Compartment.h

@@ -124,18 +124,10 @@ class CrossCompartmentKey {
         : Debuggee(debugger, referent) {}
   };
 
-  // Key under which we find debugger's Debugger.Frame for the generator call
-  // whose AbstractGeneratorObject is referent.
-  struct DebuggeeFrameGeneratorScript : Debuggee<JSScript> {
-    DebuggeeFrameGeneratorScript(NativeObject* debugger, JSScript* referent)
-        : Debuggee(debugger, referent) {}
-  };
-
-  using WrappedType =
-      mozilla::Variant<JSObject*, JSString*, DebuggeeObject, DebuggeeJSScript,
-                       DebuggeeWasmScript, DebuggeeLazyScript,
-                       DebuggeeEnvironment, DebuggeeSource,
-                       DebuggeeFrameGenerator, DebuggeeFrameGeneratorScript>;
+  using WrappedType = mozilla::Variant<JSObject*, JSString*, DebuggeeObject,
+                                       DebuggeeJSScript, DebuggeeWasmScript,
+                                       DebuggeeLazyScript, DebuggeeEnvironment,
+                                       DebuggeeSource, DebuggeeFrameGenerator>;
 
   explicit CrossCompartmentKey(JSObject* obj) : wrapped(obj) {
     MOZ_RELEASE_ASSERT(obj);
@@ -160,8 +152,6 @@ class CrossCompartmentKey {
       : wrapped(std::move(key)) {}
   explicit CrossCompartmentKey(DebuggeeFrameGenerator&& key)
       : wrapped(std::move(key)) {}
-  explicit CrossCompartmentKey(DebuggeeFrameGeneratorScript&& key)
-      : wrapped(std::move(key)) {}
   explicit CrossCompartmentKey(NativeObject* debugger, JSScript* referent)
       : wrapped(DebuggeeJSScript(debugger, referent)) {}
   explicit CrossCompartmentKey(NativeObject* debugger, LazyScript* referent)
@@ -602,23 +592,22 @@ class JS::Compartment {
                                const js::CrossCompartmentKey& wrapped,
                                const js::Value& wrapper);
 
+  js::WrapperMap::Ptr lookupWrapper(const js::CrossCompartmentKey& key) const {
+    return crossCompartmentWrappers.lookup(key);
+  }
+
   js::WrapperMap::Ptr lookupWrapper(const js::Value& wrapped) const {
-    return crossCompartmentWrappers.lookup(js::CrossCompartmentKey(wrapped));
+    return lookupWrapper(js::CrossCompartmentKey(wrapped));
   }
 
   js::WrapperMap::Ptr lookupWrapper(JSObject* obj) const {
-    return crossCompartmentWrappers.lookup(js::CrossCompartmentKey(obj));
+    return lookupWrapper(js::CrossCompartmentKey(obj));
   }
 
   void removeWrapper(js::WrapperMap::Ptr p) {
     crossCompartmentWrappers.remove(p);
   }
 
-  void removeWrapper(const js::CrossCompartmentKey& key) {
-    js::WrapperMap::Ptr p = crossCompartmentWrappers.lookup(key);
-    crossCompartmentWrappers.remove(p);
-  }
-
   bool hasNurseryAllocatedWrapperEntries(const js::CompartmentFilter& f) {
     return crossCompartmentWrappers.hasNurseryAllocatedWrapperEntries(f);
   }

js/src/vm/Debugger.cpp

@@ -9026,6 +9026,15 @@ DebuggerFrame* DebuggerFrame::create(JSContext* cx, HandleObject proto,
  *
  * Yes, officer, I definitely knew all this in advance and designed it this way
  * the first time.
+ *
+ * Note that it is not necessary to have a second cross-compartment wrapper
+ * table entry to cover the pointer to the generator's script. The wrapper table
+ * entries play two roles: they help the GC put a debugger zone in the same zone
+ * group as its debuggee, and they serve as roots when collecting the debuggee
+ * zone, but not the debugger zone. Since an AbstractGeneratorObject holds a
+ * strong reference to its callee's script (via the callee), and the AGO and the
+ * script are always in the same compartment, it suffices to add a
+ * cross-compartment wrapper table entry for the Debugger.Frame -> AGO edge.
  */
 class DebuggerFrame::GeneratorInfo {
   // An unwrapped cross-compartment reference to the generator object.
@@ -9076,7 +9085,7 @@ bool DebuggerFrame::setGenerator(JSContext* cx,
     return true;
   }
 
-  // There are five relations we must establish:
+  // There are four relations we must establish:
   //
   // 1) The DebuggerFrame must point to the AbstractGeneratorObject.
   //
@@ -9086,10 +9095,7 @@ bool DebuggerFrame::setGenerator(JSContext* cx,
   // 3) The compartment's crossCompartmentWrappers map must map this Debugger
   //    and the AbstractGeneratorObject to the DebuggerFrame.
   //
-  // 4) The compartment's crossCompartmentWrappers map must map this Debugger
-  //    and the generator's JSScript to the DebuggerFrame.
-  //
-  // 5) The generator's script's observer count must be bumped.
+  // 4) The generator's script's observer count must be bumped.
   RootedScript script(cx, genObj->callee().nonLazyScript());
   auto* info = cx->new_<GeneratorInfo>(genObj, script);
   if (!info) {
@@ -9109,17 +9115,14 @@ bool DebuggerFrame::setGenerator(JSContext* cx,
   Rooted<CrossCompartmentKey> generatorKey(
       cx, CrossCompartmentKey::DebuggeeFrameGenerator(owner()->toJSObject(),
                                                       genObj));
-  Rooted<CrossCompartmentKey> scriptKey(
-      cx, CrossCompartmentKey::DebuggeeFrameGeneratorScript(
-              owner()->toJSObject(), script));
-  auto crossCompartmentKeysGuard = MakeScopeExit([&] {
-    compartment()->removeWrapper(generatorKey);
-    compartment()->removeWrapper(scriptKey);
-  });
-  if (!compartment()->putWrapper(cx, generatorKey, ObjectValue(*this)) ||
-      !compartment()->putWrapper(cx, scriptKey, ObjectValue(*this))) {
+  if (!compartment()->putWrapper(cx, generatorKey, ObjectValue(*this))) {
     return false;
   }
+  auto crossCompartmentKeysGuard = MakeScopeExit([&] {
+    WrapperMap::Ptr generatorPtr = compartment()->lookupWrapper(generatorKey);
+    MOZ_ASSERT(generatorPtr);
+    compartment()->removeWrapper(generatorPtr);
+  });
 
   {
     AutoRealm ar(cx, script);
@@ -9144,7 +9147,7 @@ void DebuggerFrame::clearGenerator(FreeOp* fop) {
 
   GeneratorInfo* info = generatorInfo();
 
-  // 5) The generator's script's observer count must be dropped.
+  // 4) The generator's script's observer count must be dropped.
   //
   // For ordinary calls, Debugger.Frame objects drop the script's stepper count
   // when the frame is popped, but for generators, they leave the stepper count
@@ -9175,21 +9178,15 @@ void DebuggerFrame::clearGenerator(
     return;
   }
 
-  // 4) The compartment's crossCompartmentWrappers map must map this Debugger
-  //    and the generator's JSScript to the DebuggerFrame.
-  //
   // 3) The compartment's crossCompartmentWrappers map must map this Debugger
   //    and the AbstractGeneratorObject to the DebuggerFrame.
   //
   GeneratorInfo* info = generatorInfo();
-  HeapPtr<JSScript*>& generatorScript = info->generatorScript();
   CrossCompartmentKey generatorKey(CrossCompartmentKey::DebuggeeFrameGenerator(
       owner->object, &info->unwrappedGenerator()));
-  CrossCompartmentKey scriptKey(
-      CrossCompartmentKey::DebuggeeFrameGeneratorScript(owner->object,
-                                                        generatorScript));
-  compartment()->removeWrapper(generatorKey);
-  compartment()->removeWrapper(scriptKey);
+  auto generatorPtr = compartment()->lookupWrapper(generatorKey);
+  MOZ_ASSERT(generatorPtr);
+  compartment()->removeWrapper(generatorPtr);
 
   // 2) generatorFrames must no longer map the AbstractGeneratorObject to the
   // DebuggerFrame.

js/src/vm/Debugger.h

@@ -1528,7 +1528,7 @@ class DebuggerFrame : public NativeObject {
    *
    * If provided, owner must be the Debugger to which this Debugger.Frame
    * belongs; remove this frame's entry from its generatorFrames map, and clean
-   * up its cross-compartment wrapper table entries. The owner must be passed
+   * up its cross-compartment wrapper table entry. The owner must be passed
    * unless this method is being called from the Debugger.Frame's finalizer. (In
    * that case, the owner is not reliably available, and is not actually
    * necessary.)