Bug 1351501, part 2 - Preserve wrappers for non-nsISupports cycle collected weak map keys r=bzbarsky
authorAndrew McCreight <continuation@gmail.com>
Fri, 21 Sep 2018 18:20:35 +0000
changeset 493452 ef927b08625419b8033f8b20d939d8ac84c88ebb
parent 493451 ab2856e070b9de566ec676655ac42431cdb8c17f
child 493453 ae99a39d649760c62a7f83a729ff0a4a8c5cbfb5
push id9984
push userffxbld-merge
push dateMon, 15 Oct 2018 21:07:35 +0000
treeherdermozilla-beta@183d27ea8570 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbzbarsky
bugs1351501
milestone64.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1351501, part 2 - Preserve wrappers for non-nsISupports cycle collected weak map keys r=bzbarsky A C++ object that is exposed to JS can have its reflector used as a key in a weak map. Because a weak map does not keep its keys alive, this means that the reflector can be discarded if it has no other references aside from the C++ object, which will in turn remove its weak map entry. If the C++ object can be accessed again later from JS, it will get a new reflector which will have no weak map entry. This is bad because it means some internal implementation detail has resulted in data loss that is visible to JS. (Side note: this is also an issue for cross compartment wrappers, which is handled by another mechanism.) To fix this, we can preserve the wrapper of any DOM reflector used as a weak map key. This ensures that the reflector and its C++ object have the same lifetime. If a WebIDL object is not wrapper cached, that means that it cannot be accessed via C++, so we don't need to preserve the wrapper. This is currently implemented for nsISupports classes, but not other classes. For non-nsISupports classes, it would throw an error rather than silently fail. My patch adds support for non-nsISupports cycle collected objects. It turns out that the existing addProperty hook just does wrapper preservation, so we just call it for cycle collected classes. This does mean that if addProperty changes in the future to do something else, this code will need to be changed. I verified that this test fails if TryPreserveWrapper is changed to do nothing besides return true in the non-nsISuports case. Depends on D6197 Differential Revision: https://phabricator.services.mozilla.com/D6198
dom/bindings/BindingUtils.cpp
dom/bindings/Codegen.py
js/xpconnect/tests/chrome/test_paris_weakmap_keys.xul
--- a/dom/bindings/BindingUtils.cpp
+++ b/dom/bindings/BindingUtils.cpp
@@ -1147,30 +1147,47 @@ NativeInterface2JSObjectAndThrowIfFailed
   return true;
 }
 
 bool
 TryPreserveWrapper(JS::Handle<JSObject*> obj)
 {
   MOZ_ASSERT(IsDOMObject(obj));
 
+  // nsISupports objects are special cased because DOM proxies are nsISupports
+  // and have addProperty hooks that do more than wrapper preservation (so we
+  // don't want to call them).
   if (nsISupports* native = UnwrapDOMObjectToISupports(obj)) {
     nsWrapperCache* cache = nullptr;
     CallQueryInterface(native, &cache);
     if (cache) {
       cache->PreserveWrapper(native);
     }
     return true;
   }
 
-  // If this DOMClass is not cycle collected, then it isn't wrappercached,
-  // so it does not need to be preserved. If it is cycle collected, then
-  // we can't tell if it is wrappercached or not, so we just return false.
+  // The addProperty hook for WebIDL classes does wrapper preservation, and
+  // nothing else, so call it, if present.
   const DOMJSClass* domClass = GetDOMClass(obj);
-  return domClass && !domClass->mParticipant;
+  const JSClass* clasp = domClass->ToJSClass();
+  JSAddPropertyOp addProperty = clasp->getAddProperty();
+
+  // We expect all proxies to be nsISupports.
+  MOZ_RELEASE_ASSERT(!js::Valueify(clasp)->isProxy(), "Should not call addProperty for proxies.");
+
+  // The class should have an addProperty hook iff it is a CC participant.
+  MOZ_RELEASE_ASSERT(bool(domClass->mParticipant) == bool(addProperty));
+
+  if (!addProperty) {
+    return true;
+  }
+
+  JS::Rooted<jsid> dummyId(RootingCx());
+  JS::Rooted<JS::Value> dummyValue(RootingCx());
+  return addProperty(nullptr, obj, dummyId, dummyValue);
 }
 
 // Can only be called with a DOM JSClass.
 bool
 InstanceClassHasProtoAtDepth(const js::Class* clasp,
                              uint32_t protoID, uint32_t depth)
 {
   const DOMJSClass* domClass = DOMJSClass::FromJSClass(clasp);
--- a/dom/bindings/Codegen.py
+++ b/dom/bindings/Codegen.py
@@ -97,16 +97,19 @@ def idlTypeNeedsCycleCollection(type):
                                              type)
         return False
     elif type.isDictionary():
         return CGDictionary.dictionaryNeedsCycleCollection(type.inner)
     else:
         raise CycleCollectionUnsupported("Don't know whether to cycle-collect type %s" % type)
 
 
+# TryPreserveWrapper uses the addProperty hook to preserve the wrapper of
+# non-nsISupports cycle collected objects, so if wantsAddProperty is changed
+# to not cover that case then TryPreserveWrapper will need to be changed.
 def wantsAddProperty(desc):
     return (desc.concrete and desc.wrapperCache and not desc.isGlobal())
 
 
 # We'll want to insert the indent at the beginnings of lines, but we
 # don't want to indent empty lines.  So only indent lines that have a
 # non-newline character on them.
 lineStartDetector = re.compile("^(?=[^\n#])", re.MULTILINE)
@@ -1678,16 +1681,20 @@ class CGAddPropertyHook(CGAbstractClassH
                 Argument('JS::Handle<JSObject*>', 'obj'),
                 Argument('JS::Handle<jsid>', 'id'),
                 Argument('JS::Handle<JS::Value>', 'val')]
         CGAbstractClassHook.__init__(self, descriptor, ADDPROPERTY_HOOK_NAME,
                                      'bool', args)
 
     def generate_code(self):
         assert self.descriptor.wrapperCache
+        # This hook is also called by TryPreserveWrapper on non-nsISupports
+        # cycle collected objects, so if addProperty is ever changed to do
+        # anything more or less than preserve the wrapper, TryPreserveWrapper
+        # will need to be changed.
         return dedent("""
             // We don't want to preserve if we don't have a wrapper, and we
             // obviously can't preserve if we're not initialized.
             if (self && self->GetWrapperPreserveColor()) {
               PreserveWrapper(self);
             }
             return true;
             """)
--- a/js/xpconnect/tests/chrome/test_paris_weakmap_keys.xul
+++ b/js/xpconnect/tests/chrome/test_paris_weakmap_keys.xul
@@ -38,41 +38,56 @@ https://bugzilla.mozilla.org/show_bug.cg
     ok(!div_fail, "Using elem.style as a weak map key should not produce an exception.");
 
     is(live_map.get(get_div_style()), 12345, "Live map should have live style with right value before GC.");
 
   }
 
   make_live_map();
 
-  // DOMPoint is a non-nsISupports refCounted class using WebIDL bindings.
+
+  // CanvasGradient is a non-nsISupports wrapper cached class using WebIDL
+  // bindings. If we used it as a key in a weak map, then it should not be
+  // removed from the weak map as long as it remains alive.
+  let doc = new DOMParser().parseFromString("", "text/html");
+  let canv = doc.createElement("canvas");
+  let ctx = canv.getContext("2d");
 
-  // non-nsISupports cycle-collected classes should fail as weak map keys.
-  let context = new DOMPoint(1, 2, 3);
-  let contextFail = false;
-  try {
-    live_map.set(context, 2);
-  } catch (e) {
-    contextFail = true;
+  let add_non_isupports2 = function () {
+    let grad = ctx.createLinearGradient(0, 0, 0, 0);
+    ctx.strokeStyle = grad;
+
+    let gradFail = false;
+    try {
+      live_map.set(grad, 23456);
+    } catch (e) {
+      gradFail = true;
+    }
+
+    ok(!gradFail, "Using a wrapper cached non-nsISupports class as a weak map key should not produce an exception.");
+
+    is(live_map.get(grad), 23456, "Live map should have live DOMPoint with right value before GC.");
   }
 
-  ok(contextFail, "Cycle collected non-nsISupports classes aren't allowed as weak map keys.");
+  add_non_isupports2();
+
 
   /* Set up for running precise GC/CC then check the results. */
 
   SimpleTest.waitForExplicitFinish();
 
   Cu.schedulePreciseGC(function () {
     SpecialPowers.DOMWindowUtils.cycleCollect();
     SpecialPowers.DOMWindowUtils.garbageCollect();
     SpecialPowers.DOMWindowUtils.garbageCollect();
 
-    is(ChromeUtils.nondeterministicGetWeakMapKeys(live_map).length, 1,
+    is(ChromeUtils.nondeterministicGetWeakMapKeys(live_map).length, 2,
        "Live nsISupports new DOM bindings wrappercached native weak map key should not be removed.");
 
     is(live_map.get(get_div_style()), 12345, "Live map should have live style with right value after GC.");
+    is(live_map.get(ctx.strokeStyle), 23456, "Live map should have live gradient with right value after GC.");
 
     SimpleTest.finish();
   });
 
   ]]>
   </script>
 </window>