Bug 1560754: Remove DebuggeeFrameGeneratorScript. r=jorendorff
authorJim Blandy <jimb@mozilla.com>
Sat, 06 Jul 2019 00:01:24 +0000
changeset 544379 1dd4e16a1a9633c661abd2da224d1046259513e0
parent 544378 4762f5a28a7287d48d3f6c90fe7a9e7b2b8c0c10
child 544380 b591f764ab52445d8ffc278bbd6a218e888b82ba
child 545499 a8a848e015dbaca31b1884a931caab1bc729809a
push id2131
push userffxbld-merge
push dateMon, 26 Aug 2019 18:30:20 +0000
treeherdermozilla-release@b19ffb3ca153 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjorendorff
bugs1560754
milestone69.0a1
first release with
nightly linux32
1dd4e16a1a96 / 69.0a1 / 20190706093401 / files
nightly linux64
1dd4e16a1a96 / 69.0a1 / 20190706093401 / files
nightly mac
1dd4e16a1a96 / 69.0a1 / 20190706093401 / files
nightly win32
1dd4e16a1a96 / 69.0a1 / 20190706093401 / files
nightly win64
1dd4e16a1a96 / 69.0a1 / 20190706093401 / files
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
releases
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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
js/src/jit-test/tests/debug/bug1557343-2.js
js/src/vm/Compartment.h
js/src/vm/Debugger.cpp
js/src/vm/Debugger.h
new file mode 100644
--- /dev/null
+++ b/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();
--- a/js/src/vm/Compartment.h
+++ b/js/src/vm/Compartment.h
@@ -119,28 +119,20 @@ class CrossCompartmentKey {
 
   // Key under which we find debugger's Debugger.Frame for the generator call
   // whose AbstractGeneratorObject is referent.
   struct DebuggeeFrameGenerator : Debuggee<NativeObject> {
     DebuggeeFrameGenerator(NativeObject* debugger, NativeObject* referent)
         : 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);
   }
   explicit CrossCompartmentKey(JSString* str) : wrapped(str) {
     MOZ_RELEASE_ASSERT(str);
   }
   explicit CrossCompartmentKey(const JS::Value& v)
@@ -155,18 +147,16 @@ class CrossCompartmentKey {
   explicit CrossCompartmentKey(DebuggeeSource&& key)
       : wrapped(std::move(key)) {}
   explicit CrossCompartmentKey(DebuggeeEnvironment&& key)
       : wrapped(std::move(key)) {}
   explicit CrossCompartmentKey(DebuggeeWasmScript&& key)
       : 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)
       : wrapped(DebuggeeLazyScript(debugger, referent)) {}
 
   bool operator==(const CrossCompartmentKey& other) const {
     return wrapped == other.wrapped;
   }
@@ -597,33 +587,32 @@ class JS::Compartment {
                          JS::MutableHandle<JS::GCVector<JS::Value>> vec);
   MOZ_MUST_USE bool rewrap(JSContext* cx, JS::MutableHandleObject obj,
                            JS::HandleObject existing);
 
   MOZ_MUST_USE bool putWrapper(JSContext* cx,
                                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);
   }
 
   struct WrapperEnum : public js::WrapperMap::Enum {
     explicit WrapperEnum(JS::Compartment* c)
         : js::WrapperMap::Enum(c->crossCompartmentWrappers) {}
   };
--- a/js/src/vm/Debugger.cpp
+++ b/js/src/vm/Debugger.cpp
@@ -9021,16 +9021,25 @@ DebuggerFrame* DebuggerFrame::create(JSC
  *    to get zone collection groups right.
  * 3) Even if we make debugger wrapper table entries by hand, hiding
  *    cross-compartment edges as PrivateValues doesn't call post-barriers, and
  *    the generational GC won't update our pointer when the generator object
  *    gets tenured.
  *
  * 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.
   //
   // Always an object.
   //
   // This cannot be GCPtr because we are not always destructed during sweeping;
   // a Debugger.Frame's generator is also cleared when the generator returns
@@ -9071,30 +9080,27 @@ bool DebuggerFrame::setGenerator(JSConte
   Debugger::GeneratorWeakMap::AddPtr p =
       owner()->generatorFrames.lookupForAdd(genObj);
   if (p) {
     MOZ_ASSERT(p->value() == this);
     MOZ_ASSERT(&unwrappedGenerator() == genObj);
     return true;
   }
 
-  // There are five relations we must establish:
+  // There are four relations we must establish:
   //
   // 1) The DebuggerFrame must point to the AbstractGeneratorObject.
   //
   // 2) generatorFrames must map the AbstractGeneratorObject to the
   //    DebuggerFrame.
   //
   // 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) {
     ReportOutOfMemory(cx);
     return false;
   }
   auto infoGuard =
       MakeScopeExit([&] { cx->runtime()->defaultFreeOp()->delete_(info); });
@@ -9104,27 +9110,24 @@ bool DebuggerFrame::setGenerator(JSConte
     return false;
   }
   auto generatorFramesGuard =
       MakeScopeExit([&] { owner()->generatorFrames.remove(genObj); });
 
   Rooted<CrossCompartmentKey> generatorKey(
       cx, CrossCompartmentKey::DebuggeeFrameGenerator(owner()->toJSObject(),
                                                       genObj));
-  Rooted<CrossCompartmentKey> scriptKey(
-      cx, CrossCompartmentKey::DebuggeeFrameGeneratorScript(
-              owner()->toJSObject(), script));
+  if (!compartment()->putWrapper(cx, generatorKey, ObjectValue(*this))) {
+    return false;
+  }
   auto crossCompartmentKeysGuard = MakeScopeExit([&] {
-    compartment()->removeWrapper(generatorKey);
-    compartment()->removeWrapper(scriptKey);
+    WrapperMap::Ptr generatorPtr = compartment()->lookupWrapper(generatorKey);
+    MOZ_ASSERT(generatorPtr);
+    compartment()->removeWrapper(generatorPtr);
   });
-  if (!compartment()->putWrapper(cx, generatorKey, ObjectValue(*this)) ||
-      !compartment()->putWrapper(cx, scriptKey, ObjectValue(*this))) {
-    return false;
-  }
 
   {
     AutoRealm ar(cx, script);
     if (!script->incrementGeneratorObserverCount(cx)) {
       return false;
     }
   }
 
@@ -9139,17 +9142,17 @@ bool DebuggerFrame::setGenerator(JSConte
 
 void DebuggerFrame::clearGenerator(FreeOp* fop) {
   if (!hasGenerator()) {
     return;
   }
 
   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
   // incremented across suspensions. This means that, whereas ordinary calls
   // never need to drop the stepper count from the D.F finalizer, generator
   // calls may.
   HeapPtr<JSScript*>& generatorScript = info->generatorScript();
   if (!IsAboutToBeFinalized(&generatorScript)) {
@@ -9170,31 +9173,25 @@ void DebuggerFrame::clearGenerator(FreeO
 
 void DebuggerFrame::clearGenerator(
     FreeOp* fop, Debugger* owner,
     Debugger::GeneratorWeakMap::Enum* maybeGeneratorFramesEnum) {
   if (!hasGenerator()) {
     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.
   if (maybeGeneratorFramesEnum) {
     maybeGeneratorFramesEnum->removeFront();
   } else {
     owner->generatorFrames.remove(&info->unwrappedGenerator());
   }
--- a/js/src/vm/Debugger.h
+++ b/js/src/vm/Debugger.h
@@ -1523,17 +1523,17 @@ class DebuggerFrame : public NativeObjec
   MOZ_MUST_USE bool setGenerator(JSContext* cx,
                                  Handle<AbstractGeneratorObject*> genObj);
 
   /*
    * Undo the effects of a prior call to setGenerator.
    *
    * 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.)
    *
    * 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.