Update DefineClass comment

Author: Gabriel Schulhof

napi-inl.h

@@ -2823,6 +2823,15 @@ ObjectWrap<T>::DefineClass(Napi::Env env,
                            const napi_property_descriptor* props,
                            void* data) {
   napi_status status;
+
+  // Before defining the class we can replace static method property descriptors
+  // with value property descriptors such that the value is a function-valued
+  // `napi_value` created with `CreateFunction()`. Note that we have to break
+  // constness to do this.
+  //
+  // This replacement could be made for instance methods as well, but V8 aborts
+  // if we do that, because it expects methods defined on the prototype template
+  // to have `FunctionTemplate`s.
   for (size_t index = 0; index < props_count; index++) {
     napi_property_descriptor* prop =
         const_cast<napi_property_descriptor*>(&props[index]);
@@ -2858,6 +2867,41 @@ ObjectWrap<T>::DefineClass(Napi::Env env,
                              &value);
   NAPI_THROW_IF_FAILED(env, status, Function());
 
+  // After defining the class we iterate once more over the property descriptors
+  // and attach the data associated with accessors and instance methods to the
+  // newly created JavaScript class.
+  //
+  // TODO: On some engines it might be possible to detach instance methods from
+  // the prototype. This would mean that the detached prototype method would
+  // become a plain function and so its data should be associated with itself
+  // rather than with the class. If this is the case then in this loop, and for
+  // prototype methods, we should retrieve the `napi_value` representing the
+  // resulting function and attach the data to *it* rather than the class.
+  //
+  // IOW if in JavaScript one does this,
+  //
+  // let MyClass = binding.defineMyClass();
+  // const someMethod = MyClass.prototype.someMethod;
+  // delete MyClass;
+  // someMethod();
+  //
+  // then the class, onto which we attached the data, would be gone, and the
+  // data that will be passed to the native implementation of `someMethod()`
+  // would be stale and would cause a segfault if accessed, and so in the
+  // following loop, we may have to
+  //
+  // napi_get_named_property(env, value, "prototype", &proto);
+  // once at the top, and then, for each property, if it is an instance method,
+  // napi_get_named_property(env, value, prop->utf8name, &proto_method);
+  // or
+  // napi_get_property(env, prop->name, &proto_method);
+  // and then
+  // Napi::details::AttachData(env,
+  //                           proto_method,
+  //                           static_cast<...*>(prop->data));
+  //
+  // instead of attaching the data to the class. The downside is that all this
+  // retrieving of prototype properties would be very expensive.
   for (size_t idx = 0; idx < props_count; idx++) {
     const napi_property_descriptor* prop = &props[idx];
 
@@ -2872,17 +2916,19 @@ ObjectWrap<T>::DefineClass(Napi::Env env,
       status = Napi::details::AttachData(env,
                           value,
                           static_cast<InstanceAccessorCallbackData*>(prop->data));
+      NAPI_THROW_IF_FAILED(env, status, Function());
     } else if (prop->method != nullptr && !(prop->attributes & napi_static)) {
       if (prop->method == T::InstanceVoidMethodCallbackWrapper) {
         status = Napi::details::AttachData(env,
                       value,
                       static_cast<InstanceVoidMethodCallbackData*>(prop->data));
+        NAPI_THROW_IF_FAILED(env, status, Function());
       } else if (prop->method == T::InstanceMethodCallbackWrapper) {
         status = Napi::details::AttachData(env,
                           value,
                           static_cast<InstanceMethodCallbackData*>(prop->data));
+        NAPI_THROW_IF_FAILED(env, status, Function());
       }
-      NAPI_THROW_IF_FAILED(env, status, Function());
     }
   }