Gin: Make it easier to implement Wrappable

gin/wrappable.h

Index: gin/wrappable.h
diff --git a/gin/wrappable.h b/gin/wrappable.h
index 741bf8ff6d11b72ef7a3ffc4297868aa2346adf4..84187048669643e5a82eacf9186cc6249f5a2e0c 100644
--- a/gin/wrappable.h
+++ b/gin/wrappable.h
@@ -11,62 +11,76 @@
 
 namespace gin {
 
-// Wrappable is an abstract base class for C++ objects that have cooresponding
-// v8 wrapper objects. To retain a Wrappable object on the stack, use a
-// gin::Handle.
-class Wrappable {
- public:
-  // Subclasses must return the WrapperInfo object associated with the
-  // v8::ObjectTemplate for their subclass. When creating a v8 wrapper for
-  // this object, we'll look up the appropriate v8::ObjectTemplate in the
-  // PerIsolateData using this WrapperInfo pointer.
-  virtual WrapperInfo* GetWrapperInfo() = 0;
+template<typename T>
+struct WrappableTraits {
+  static WrapperInfo kWrapperInfo;
+};
 
-  // Subclasses much also contain a static member variable named |kWrapperInfo|
-  // of type WrapperInfo:
-  //
-  //   static WrapperInfo kWrapperInfo;
-  //
-  // If |obj| is a concrete instance of the subclass, then obj->GetWrapperInfo()
-  // must return &kWrapperInfo.
-  //
-  // We use both the dynamic |GetWrapperInfo| function and the static
-  // |kWrapperInfo| member variable during wrapping and the unwrapping. During
-  // wrapping, we use GetWrapperInfo() to make sure we use the correct
-  // v8::ObjectTemplate for the object regardless of the declared C++ type.
-  // During unwrapping, we use the static member variable to prevent type errors
-  // during the downcast from Wrappable to the subclass.
+template<typename T>
+WrapperInfo WrappableTraits<T>::kWrapperInfo = {
+  gin::kEmbedderNativeGin
+};
+
+struct WrappableTraitsTest {
+  static WrapperInfo* GetWrapperInfo();
+};
+
+
+// Wrappable is a base class for C++ objects that have cooresponding v8 wrapper
+// objects. To retain a Wrappable object on the stack, use a gin::Handle.
+//
+// Usage:
+// class MyClass : Wrappable<MyClass> {
+//   ...
+// };
+//
+// Subclasses should have private constructors and expose a static Create
+// function that returns a gin::Handle. Forcing creators through this static
+// Create function will enforce that clients actually create a wrapper for the
+// object. If clients fail to create a wrapper for a wrappable object, the
+// object will leak because we use the weak callback from the wrapper as the
+// signal to delete the wrapped object.
+template<typename T>
+class Wrappable;
 
+
+class WrappableBase {
+ public:
   // Retrieve (or create) the v8 wrapper object cooresponding to this object.
   // To customize the wrapper created for a subclass, override GetWrapperInfo()
   // instead of overriding this function.
   v8::Handle<v8::Object> GetWrapper(v8::Isolate* isolate);
 
-  // Subclasses should have private constructors and expose a static Create
-  // function that returns a gin::Handle. Forcing creators through this static
-  // Create function will enforce that clients actually create a wrapper for
-  // the object. If clients fail to create a wrapper for a wrappable object,
-  // the object will leak because we use the weak callback from the wrapper
-  // as the signal to delete the wrapped object.
+  virtual WrapperInfo* GetWrapperInfo() = 0;
 
  protected:
-  Wrappable();
-  virtual ~Wrappable();
+  WrappableBase();
+  virtual ~WrappableBase();
 
  private:
   static void WeakCallback(
-      const v8::WeakCallbackData<v8::Object, Wrappable>& data);
+      const v8::WeakCallbackData<v8::Object, WrappableBase>& data);
   v8::Handle<v8::Object> CreateWrapper(v8::Isolate* isolate);
 
   v8::Persistent<v8::Object> wrapper_;  // Weak
 
-  DISALLOW_COPY_AND_ASSIGN(Wrappable);
+  DISALLOW_COPY_AND_ASSIGN(WrappableBase);
+};
+
+
+template<typename T>
+class Wrappable : public WrappableBase {
+ public:
+  virtual WrapperInfo* GetWrapperInfo() {

[Aaron Boodman, 2013/12/06 22:40:55]

I am kind of grumpy about the extra inheritance and especially this virtual call
which will be on the critical path for every JS->C++ call.

I can fix it, but I guess I'm not clear if that would defeat some security thing
you were trying to do. What exactly is the point again of the
CHECK(result->GetWrapperInfo() == this) in FromV8()? If that fails, it seems
like it indicates one of either:

a) Coding bug on our part.
b) Malicious JavaScript/content.

But why are we taking extra precaution against either of those for this
particular code -- that's a general concern with all content inside the sandbox.

[abarth-chromium, 2013/12/06 23:03:10]

It's a security check to catch a class of use-after-free vulnerabilities.  The
security team added something similar to the Blink bindings.  We can remove it
for now and consider re-adding it in the future once we can measure any
performance impact.

+    return &WrappableTraits<T>::kWrapperInfo;
+  }
 };
 
+
 // This converter handles any subclass of Wrappable.
 template<typename T>
 struct Converter<T*, typename base::enable_if<
-                      base::is_convertible<T*, Wrappable*>::value>::type> {
+                       base::is_convertible<T*, Wrappable<T>*>::value>::type> {
   static v8::Handle<v8::Value> ToV8(v8::Isolate* isolate, T* val) {
     return val->GetWrapper(isolate);
   }
@@ -85,7 +99,7 @@ struct Converter<T*, typename base::enable_if<
 
     // If this fails, the object is managed by Gin, but it's not wrapping an
     // instance of T.
-    if (info != &T::kWrapperInfo)
+    if (info != &WrappableTraits<T>::kWrapperInfo)
       return false;
 
     void* pointer = obj->GetAlignedPointerFromInternalField(kEncodedValueIndex);