Bug 1360523 - Define number of reserved slots explicitly for each proxy js::Class. r=bz
authorJan de Mooij <jdemooij@mozilla.com>
Sat, 29 Apr 2017 14:41:49 +0200
changeset 355773 1d5d4015f0c19d5da7c76b5e134eb227431a6356
parent 355772 278358e80893a5fccbd6ff6aa11585b81ecc30e8
child 355774 3ff1f94e57675b0f66dac043e1c78b7965c062c5
push id89739
push userjandemooij@gmail.com
push dateSat, 29 Apr 2017 12:42:52 +0000
treeherdermozilla-inbound@1d5d4015f0c1 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbz
bugs1360523
milestone55.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 1360523 - Define number of reserved slots explicitly for each proxy js::Class. r=bz
dom/base/WindowNamedPropertiesHandler.cpp
dom/base/nsGlobalWindow.cpp
dom/bindings/Codegen.py
js/public/Proxy.h
js/src/jsapi-tests/testBug604087.cpp
js/src/jsfriendapi.h
js/src/proxy/Proxy.cpp
js/src/vm/NativeObject.h
js/src/vm/ProxyObject.cpp
--- a/dom/base/WindowNamedPropertiesHandler.cpp
+++ b/dom/base/WindowNamedPropertiesHandler.cpp
@@ -323,19 +323,22 @@ const NativePropertyHooks sWindowNamedPr
   EnumerateWindowNamedProperties,
   nullptr,
   { nullptr, nullptr },
   prototypes::id::_ID_Count,
   constructors::id::_ID_Count,
   nullptr
 } };
 
+// Note that this class doesn't need any reserved slots, but SpiderMonkey
+// asserts all proxy classes have at least one reserved slot.
 static const DOMIfaceAndProtoJSClass WindowNamedPropertiesClass = {
   PROXY_CLASS_DEF("WindowProperties",
-                  JSCLASS_IS_DOMIFACEANDPROTOJSCLASS),
+                  JSCLASS_IS_DOMIFACEANDPROTOJSCLASS |
+                  JSCLASS_HAS_RESERVED_SLOTS(1)),
   eNamedPropertiesObject,
   false,
   prototypes::id::_ID_Count,
   0,
   sWindowNamedPropertiesNativePropertyHooks,
   "[object WindowProperties]",
   EventTargetBinding::GetProtoObject
 };
--- a/dom/base/nsGlobalWindow.cpp
+++ b/dom/base/nsGlobalWindow.cpp
@@ -1138,19 +1138,22 @@ protected:
   bool AppendIndexedPropertyNames(JSContext *cx, JSObject *proxy,
                                   JS::AutoIdVector &props) const;
 };
 
 static const js::ClassExtension OuterWindowProxyClassExtension = PROXY_MAKE_EXT(
     nsOuterWindowProxy::ObjectMoved
 );
 
+// Give OuterWindowProxyClass 2 reserved slots, like the other wrappers, so
+// JSObject::swap can swap it with CrossCompartmentWrappers without requiring
+// malloc.
 const js::Class OuterWindowProxyClass = PROXY_CLASS_WITH_EXT(
     "Proxy",
-    0, /* additional class flags */
+    JSCLASS_HAS_RESERVED_SLOTS(2), /* additional class flags */
     &OuterWindowProxyClassExtension);
 
 const char *
 nsOuterWindowProxy::className(JSContext *cx, JS::Handle<JSObject*> proxy) const
 {
     MOZ_ASSERT(js::IsProxy(proxy));
 
     return "Window";
--- a/dom/bindings/Codegen.py
+++ b/dom/bindings/Codegen.py
@@ -523,17 +523,19 @@ class CGDOMProxyJSClass(CGThing):
     def __init__(self, descriptor):
         CGThing.__init__(self)
         self.descriptor = descriptor
 
     def declare(self):
         return ""
 
     def define(self):
-        flags = ["JSCLASS_IS_DOMJSCLASS"]
+        # We need one reserved slot (DOM_OBJECT_SLOT).
+        flags = ["JSCLASS_IS_DOMJSCLASS",
+                 "JSCLASS_HAS_RESERVED_SLOTS(1)"]
         # We don't use an IDL annotation for JSCLASS_EMULATES_UNDEFINED because
         # we don't want people ever adding that to any interface other than
         # HTMLAllCollection.  So just hardcode it here.
         if self.descriptor.interface.identifier.name == "HTMLAllCollection":
             flags.append("JSCLASS_EMULATES_UNDEFINED")
         objectMovedHook = OBJECT_MOVED_HOOK_NAME if self.descriptor.wrapperCache else 'nullptr'
         return fill(
             """
--- a/js/public/Proxy.h
+++ b/js/public/Proxy.h
@@ -676,11 +676,62 @@ assertEnteredPolicy(JSContext* cx, JSObj
 inline void assertEnteredPolicy(JSContext* cx, JSObject* obj, jsid id,
                                 BaseProxyHandler::Action act)
 {}
 #endif
 
 extern JS_FRIEND_API(JSObject*)
 InitProxyClass(JSContext* cx, JS::HandleObject obj);
 
+extern JS_FRIEND_DATA(const js::ClassOps) ProxyClassOps;
+extern JS_FRIEND_DATA(const js::ClassExtension) ProxyClassExtension;
+extern JS_FRIEND_DATA(const js::ObjectOps) ProxyObjectOps;
+
+/*
+ * Helper Macros for creating JSClasses that function as proxies.
+ *
+ * NB: The macro invocation must be surrounded by braces, so as to
+ *     allow for potential JSClass extensions.
+ */
+#define PROXY_MAKE_EXT(objectMoved)                                     \
+    {                                                                   \
+        js::proxy_WeakmapKeyDelegate,                                   \
+        objectMoved                                                     \
+    }
+
+template <unsigned Flags>
+constexpr unsigned
+CheckProxyFlags()
+{
+    // For now assert each Proxy Class has at least 1 reserved slot. This is
+    // not a hard requirement, but helps catch Classes that need an explicit
+    // JSCLASS_HAS_RESERVED_SLOTS since bug 1360523.
+    static_assert(((Flags >> JSCLASS_RESERVED_SLOTS_SHIFT) & JSCLASS_RESERVED_SLOTS_MASK) > 0,
+                  "Proxy Classes must have at least 1 reserved slot");
+
+    // ProxyValueArray must fit inline in the object, so assert the number of
+    // slots does not exceed MAX_FIXED_SLOTS.
+    static_assert((offsetof(js::detail::ProxyValueArray, reservedSlots) / sizeof(Value)) +
+                  ((Flags >> JSCLASS_RESERVED_SLOTS_SHIFT) & JSCLASS_RESERVED_SLOTS_MASK)
+                  <= shadow::Object::MAX_FIXED_SLOTS,
+                  "ProxyValueArray size must not exceed max JSObject size");
+    return Flags;
+}
+
+#define PROXY_CLASS_WITH_EXT(name, flags, extPtr)                                       \
+    {                                                                                   \
+        name,                                                                           \
+        js::Class::NON_NATIVE |                                                         \
+            JSCLASS_IS_PROXY |                                                          \
+            JSCLASS_DELAY_METADATA_BUILDER |                                            \
+            js::CheckProxyFlags<flags>(),                                               \
+        &js::ProxyClassOps,                                                             \
+        JS_NULL_CLASS_SPEC,                                                             \
+        extPtr,                                                                         \
+        &js::ProxyObjectOps                                                             \
+    }
+
+#define PROXY_CLASS_DEF(name, flags) \
+  PROXY_CLASS_WITH_EXT(name, flags, &js::ProxyClassExtension)
+
 } /* namespace js */
 
 #endif /* js_Proxy_h */
--- a/js/src/jsapi-tests/testBug604087.cpp
+++ b/js/src/jsapi-tests/testBug604087.cpp
@@ -15,17 +15,17 @@
 #include "vm/ProxyObject.h"
 
 static const js::ClassExtension OuterWrapperClassExtension = PROXY_MAKE_EXT(
     nullptr  /* objectMoved */
 );
 
 const js::Class OuterWrapperClass = PROXY_CLASS_WITH_EXT(
     "Proxy",
-    0, /* additional class flags */
+    JSCLASS_HAS_RESERVED_SLOTS(1), /* additional class flags */
     &OuterWrapperClassExtension);
 
 static JSObject*
 wrap(JSContext* cx, JS::HandleObject toWrap, JS::HandleObject target)
 {
     JSAutoCompartment ac(cx, target);
     JS::RootedObject wrapper(cx, toWrap);
     if (!JS_WrapObject(cx, &wrapper))
--- a/js/src/jsfriendapi.h
+++ b/js/src/jsfriendapi.h
@@ -338,49 +338,16 @@ struct JSFunctionSpecWithHelp {
 #define JS_FS_HELP_END                                                        \
     {nullptr, nullptr, 0, 0, nullptr, nullptr}
 
 extern JS_FRIEND_API(bool)
 JS_DefineFunctionsWithHelp(JSContext* cx, JS::HandleObject obj, const JSFunctionSpecWithHelp* fs);
 
 namespace js {
 
-extern JS_FRIEND_DATA(const js::ClassOps) ProxyClassOps;
-extern JS_FRIEND_DATA(const js::ClassExtension) ProxyClassExtension;
-extern JS_FRIEND_DATA(const js::ObjectOps) ProxyObjectOps;
-
-/*
- * Helper Macros for creating JSClasses that function as proxies.
- *
- * NB: The macro invocation must be surrounded by braces, so as to
- *     allow for potential JSClass extensions.
- */
-#define PROXY_MAKE_EXT(objectMoved)                                     \
-    {                                                                   \
-        js::proxy_WeakmapKeyDelegate,                                   \
-        objectMoved                                                     \
-    }
-
-#define PROXY_CLASS_WITH_EXT(name, flags, extPtr)                                       \
-    {                                                                                   \
-        name,                                                                           \
-        js::Class::NON_NATIVE |                                                         \
-            JSCLASS_IS_PROXY |                                                          \
-            JSCLASS_DELAY_METADATA_BUILDER |                                            \
-            JSCLASS_HAS_RESERVED_SLOTS(2) |                                             \
-            flags,                                                                      \
-        &js::ProxyClassOps,                                                             \
-        JS_NULL_CLASS_SPEC,                                                             \
-        extPtr,                                                                         \
-        &js::ProxyObjectOps                                                             \
-    }
-
-#define PROXY_CLASS_DEF(name, flags) \
-  PROXY_CLASS_WITH_EXT(name, flags, &js::ProxyClassExtension)
-
 extern JS_FRIEND_API(JSObject*)
 proxy_WeakmapKeyDelegate(JSObject* obj);
 
 /**
  * A class of objects that return source code on demand.
  *
  * When code is compiled with setSourceIsLazy(true), SpiderMonkey doesn't
  * retain the source code (and doesn't do lazy bytecode generation). If we ever
@@ -551,16 +518,18 @@ public:
  * depending on the object's specific layout.
  */
 struct Object {
     shadow::ObjectGroup* group;
     shadow::Shape*      shape;
     JS::Value*          slots;
     void*               _1;
 
+    static const size_t MAX_FIXED_SLOTS = 16;
+
     size_t numFixedSlots() const { return shape->slotInfo >> Shape::FIXED_SLOTS_SHIFT; }
     JS::Value* fixedSlots() const {
         return (JS::Value*)(uintptr_t(this) + sizeof(shadow::Object));
     }
 
     JS::Value& slotRef(size_t slot) const {
         size_t nfixed = numFixedSlots();
         if (slot < nfixed)
--- a/js/src/proxy/Proxy.cpp
+++ b/js/src/proxy/Proxy.cpp
@@ -777,17 +777,19 @@ const ObjectOps js::ProxyObjectOps = {
     proxy_DeleteProperty,
     Proxy::watch, Proxy::unwatch,
     Proxy::getElements,
     nullptr,  /* enumerate */
     Proxy::fun_toString
 };
 
 const Class js::ProxyObject::proxyClass =
-    PROXY_CLASS_DEF("Proxy", JSCLASS_HAS_CACHED_PROTO(JSProto_Proxy));
+    PROXY_CLASS_DEF("Proxy",
+                    JSCLASS_HAS_CACHED_PROTO(JSProto_Proxy) |
+                    JSCLASS_HAS_RESERVED_SLOTS(2));
 
 const Class* const js::ProxyClassPtr = &js::ProxyObject::proxyClass;
 
 JS_FRIEND_API(JSObject*)
 js::NewProxyObject(JSContext* cx, const BaseProxyHandler* handler, HandleValue priv, JSObject* proto_,
                    const ProxyOptions& options)
 {
     if (options.lazyProto()) {
--- a/js/src/vm/NativeObject.h
+++ b/js/src/vm/NativeObject.h
@@ -871,17 +871,17 @@ class NativeObject : public ShapedObject
     }
 
     void initSlotUnchecked(uint32_t slot, const Value& value) {
         getSlotAddressUnchecked(slot)->init(this, HeapSlot::Slot, slot, value);
     }
 
     // MAX_FIXED_SLOTS is the biggest number of fixed slots our GC
     // size classes will give an object.
-    static const uint32_t MAX_FIXED_SLOTS = 16;
+    static const uint32_t MAX_FIXED_SLOTS = shadow::Object::MAX_FIXED_SLOTS;
 
   protected:
     MOZ_ALWAYS_INLINE bool updateSlotsForSpan(JSContext* cx, size_t oldSpan, size_t newSpan);
 
   private:
     void prepareElementRangeForOverwrite(size_t start, size_t end) {
         MOZ_ASSERT(end <= getDenseInitializedLength());
         MOZ_ASSERT(!denseElementsAreCopyOnWrite());
--- a/js/src/vm/ProxyObject.cpp
+++ b/js/src/vm/ProxyObject.cpp
@@ -15,16 +15,20 @@
 using namespace js;
 
 static gc::AllocKind
 GetProxyGCObjectKind(const Class* clasp, const BaseProxyHandler* handler, const Value& priv)
 {
     MOZ_ASSERT(clasp->isProxy());
 
     uint32_t nreserved = JSCLASS_RESERVED_SLOTS(clasp);
+
+    // For now assert each Proxy Class has at least 1 reserved slot. This is
+    // not a hard requirement, but helps catch Classes that need an explicit
+    // JSCLASS_HAS_RESERVED_SLOTS since bug 1360523.
     MOZ_ASSERT(nreserved > 0);
 
     MOZ_ASSERT(js::detail::ProxyValueArray::sizeOf(nreserved) % sizeof(Value) == 0,
                "ProxyValueArray must be a multiple of Value");
 
     uint32_t nslots = js::detail::ProxyValueArray::sizeOf(nreserved) / sizeof(Value);
     MOZ_ASSERT(nslots <= NativeObject::MAX_FIXED_SLOTS);