Remove the SetExternalStringDiposeCallback API

Changed the disposal of external string resources to call a virtual Dispose method on the resource. The default inplementation of Dispose deletes the object and will capture the delete operator matching the new operator used to allocate the object.
Review URL: http://codereview.chromium.org/2658008

git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@4816 ce2b1a6d-e550-0410-aec6-3dcde31c8c00

include/v8.h

@@ -134,6 +134,7 @@ namespace internal {
 
 class Arguments;
 class Object;
+class Heap;
 class Top;
 
 }
@@ -1037,12 +1038,24 @@ class V8EXPORT String : public Primitive {
   class V8EXPORT ExternalStringResourceBase {
    public:
     virtual ~ExternalStringResourceBase() {}
+
    protected:
     ExternalStringResourceBase() {}
+
+    /**
+     * Internally V8 will call this Dispose method when the external string
+     * resource is no longer needed. The default implementation will use the
+     * delete operator. This method can be overridden in subclasses to
+     * control how allocated external string resources are disposed.
+     */
+    virtual void Dispose() { delete this; }
+
    private:
     // Disallow copying and assigning.
     ExternalStringResourceBase(const ExternalStringResourceBase&);
     void operator=(const ExternalStringResourceBase&);
+
+    friend class v8::internal::Heap;
   };
 
   /**
@@ -1059,10 +1072,17 @@ class V8EXPORT String : public Primitive {
      * buffer.
      */
     virtual ~ExternalStringResource() {}
-    /** The string data from the underlying buffer.*/
+
+    /**
+     * The string data from the underlying buffer.
+     */
     virtual const uint16_t* data() const = 0;
-    /** The length of the string. That is, the number of two-byte characters.*/
+
+    /**
+     * The length of the string. That is, the number of two-byte characters.
+     */
     virtual size_t length() const = 0;
+
    protected:
     ExternalStringResource() {}
   };
@@ -1134,12 +1154,10 @@ class V8EXPORT String : public Primitive {
   /**
    * Creates a new external string using the data defined in the given
    * resource. When the external string is no longer live on V8's heap the
-   * resource will be disposed. If a disposal callback has been set using
-   * SetExternalStringDiposeCallback this callback will be called to dispose
-   * the resource. Otherwise, V8 will dispose the resource using the C++ delete
-   * operator. The caller of this function should not otherwise delete or
-   * modify the resource. Neither should the underlying buffer be deallocated
-   * or modified except through the destructor of the external string resource.
+   * resource will be disposed by calling its Dispose method. The caller of
+   * this function should not otherwise delete or modify the resource. Neither
+   * should the underlying buffer be deallocated or modified except through the
+   * destructor of the external string resource.
    */
   static Local<String> NewExternal(ExternalStringResource* resource);
 
@@ -1157,12 +1175,10 @@ class V8EXPORT String : public Primitive {
   /**
    * Creates a new external string using the ascii data defined in the given
    * resource. When the external string is no longer live on V8's heap the
-   * resource will be disposed. If a disposal callback has been set using
-   * SetExternalStringDiposeCallback this callback will be called to dispose
-   * the resource. Otherwise, V8 will dispose the resource using the C++ delete
-   * operator. The caller of this function should not otherwise delete or
-   * modify the resource. Neither should the underlying buffer be deallocated
-   * or modified except through the destructor of the external string resource.
+   * resource will be disposed by calling its Dispose method. The caller of
+   * this function should not otherwise delete or modify the resource. Neither
+   * should the underlying buffer be deallocated or modified except through the
+   * destructor of the external string resource.
    */
   static Local<String> NewExternal(ExternalAsciiStringResource* resource);
 
@@ -1262,10 +1278,6 @@ class V8EXPORT String : public Primitive {
 };
 
 
-typedef void (*ExternalStringDiposeCallback)
-    (String::ExternalStringResourceBase* resource);
-
-
 /**
  * A JavaScript number value (ECMA-262, 4.3.20)
  */
@@ -2482,15 +2494,6 @@ class V8EXPORT V8 {
    */
   static void RemoveMessageListeners(MessageCallback that);
 
-  /**
-   * Set a callback to be called when an external string is no longer live on
-   * V8's heap. The resource will no longer be needed by V8 and the embedder
-   * can dispose of if. If this callback is not set V8 will free the resource
-   * using the C++ delete operator.
-   */
-  static void SetExternalStringDiposeCallback(
-      ExternalStringDiposeCallback that);
-
   /**
    * Sets V8 flags from a string.
    */

src/api.cc

@@ -3698,14 +3698,6 @@ void V8::RemoveMessageListeners(MessageCallback that) {
 }
 
 
-void V8::SetExternalStringDiposeCallback(
-    ExternalStringDiposeCallback callback) {
-  if (IsDeadCheck("v8::V8::SetExternalStringDiposeCallback()"))
-    return;
-  i::Heap::SetExternalStringDiposeCallback(callback);
-}
-
-
 void V8::SetCounterFunction(CounterLookupCallback callback) {
   if (IsDeadCheck("v8::V8::SetCounterFunction()")) return;
   i::StatsTable::SetCounterFunction(callback);

src/heap-inl.h

@@ -118,11 +118,9 @@ void Heap::FinalizeExternalString(String* string) {
           ExternalString::kResourceOffset -
           kHeapObjectTag);
 
-  // Dispose of the C++ object.
-  if (external_string_dispose_callback_ != NULL) {
-    external_string_dispose_callback_(*resource_addr);
-  } else {
-    delete *resource_addr;
+  // Dispose of the C++ object if it has not already been disposed.
+  if (*resource_addr != NULL) {
+    (*resource_addr)->Dispose();
   }
 
   // Clear the resource pointer in the string.

src/heap.cc

@@ -98,8 +98,6 @@ size_t Heap::code_range_size_ = 0;
 // set up by ConfigureHeap otherwise.
 int Heap::reserved_semispace_size_ = Heap::max_semispace_size_;
 
-ExternalStringDiposeCallback Heap::external_string_dispose_callback_ = NULL;
-
 List<Heap::GCPrologueCallbackPair> Heap::gc_prologue_callbacks_;
 List<Heap::GCEpilogueCallbackPair> Heap::gc_epilogue_callbacks_;
 

src/heap.h

@@ -690,11 +690,6 @@ class Heap : public AllStatic {
   static bool GarbageCollectionGreedyCheck();
 #endif
 
-  static void SetExternalStringDiposeCallback(
-      ExternalStringDiposeCallback callback) {
-    external_string_dispose_callback_ = callback;
-  }
-
   static void AddGCPrologueCallback(
       GCEpilogueCallback callback, GCType gc_type_filter);
   static void RemoveGCPrologueCallback(GCEpilogueCallback callback);
@@ -1143,9 +1138,6 @@ class Heap : public AllStatic {
   // any string when looked up in properties.
   static String* hidden_symbol_;
 
-  static ExternalStringDiposeCallback
-       external_string_dispose_callback_;
-
   // GC callback function, called before and after mark-compact GC.
   // Allocations in the callback function are disallowed.
   struct GCPrologueCallbackPair {

test/cctest/test-api.cc

@@ -612,30 +612,33 @@ THREADED_TEST(ScavengeExternalAsciiString) {
 }
 
 
-static int dispose_count = 0;
-static void DisposeExternalStringCount(
-    String::ExternalStringResourceBase* resource) {
-  dispose_count++;
-}
+class TestAsciiResourceWithDisposeControl: public TestAsciiResource {
+ public:
+  static int dispose_calls;
+
+  TestAsciiResourceWithDisposeControl(const char* data, bool dispose)
+      : TestAsciiResource(data),
+        dispose_(dispose) { }
+
+  void Dispose() {
+    ++dispose_calls;
+    if (dispose_) delete this;
+  }
+ private:
+  bool dispose_;
+};
 
 
-static void DisposeExternalStringDeleteAndCount(
-    String::ExternalStringResourceBase* resource) {
-  delete resource;
-  dispose_count++;
-}
+int TestAsciiResourceWithDisposeControl::dispose_calls = 0;
 
 
-TEST(ExternalStringWithResourceDisposeCallback) {
+TEST(ExternalStringWithDisposeHandling) {
   const char* c_source = "1 + 2 * 3";
 
-  // Set an external string collected callback which does not delete the object.
-  v8::V8::SetExternalStringDiposeCallback(DisposeExternalStringCount);
-
   // Use a stack allocated external string resource allocated object.
-  dispose_count = 0;
   TestAsciiResource::dispose_count = 0;
-  TestAsciiResource res_stack(i::StrDup(c_source));
+  TestAsciiResourceWithDisposeControl::dispose_calls = 0;
+  TestAsciiResourceWithDisposeControl res_stack(i::StrDup(c_source), false);
   {
     v8::HandleScope scope;
     LocalContext env;
@@ -649,16 +652,14 @@ TEST(ExternalStringWithResourceDisposeCallback) {
   }
   v8::internal::CompilationCache::Clear();
   v8::internal::Heap::CollectAllGarbage(false);
-  CHECK_EQ(1, dispose_count);
+  CHECK_EQ(1, TestAsciiResourceWithDisposeControl::dispose_calls);
   CHECK_EQ(0, TestAsciiResource::dispose_count);
 
-  // Set an external string collected callback which does delete the object.
-  v8::V8::SetExternalStringDiposeCallback(DisposeExternalStringDeleteAndCount);
-
   // Use a heap allocated external string resource allocated object.
-  dispose_count = 0;
   TestAsciiResource::dispose_count = 0;
-  TestAsciiResource* res_heap = new TestAsciiResource(i::StrDup(c_source));
+  TestAsciiResourceWithDisposeControl::dispose_calls = 0;
+  TestAsciiResource* res_heap =
+      new TestAsciiResourceWithDisposeControl(i::StrDup(c_source), true);
   {
     v8::HandleScope scope;
     LocalContext env;
@@ -672,7 +673,7 @@ TEST(ExternalStringWithResourceDisposeCallback) {
   }
   v8::internal::CompilationCache::Clear();
   v8::internal::Heap::CollectAllGarbage(false);
-  CHECK_EQ(1, dispose_count);
+  CHECK_EQ(1, TestAsciiResourceWithDisposeControl::dispose_calls);
   CHECK_EQ(1, TestAsciiResource::dispose_count);
 }