Changeset 63751 in webkit

Timestamp:

Jul 20, 2010 10:20:49 AM (14 years ago)

Author:

antonm@chromium.org

Message:

2010-07-20 Anton Muhin <​antonm@chromium.org>

Reviewed by Adam Barth.

[v8] Allow handles to be disposed and WebKit objects to be dereferenced even if their map is already destroyed.
​https://bugs.webkit.org/show_bug.cgi?id=42634

Currently DOMDataStore could be destroyed even if it has some mappings (it gets destroyed
when its isolated context gets GCed). However in this case, handles allocated for
such objects would never be disposed as we require presence of mapping from wrapped
WebKit object to handle being collected in the map and now map is gone. That leads to
zombie objects in both WebKit (wrapped WebKit object doesn't get dereferenced) and V8
(both handle and V8 wrapper object could not be destroyed).

See ​http://code.google.com/p/chromium/issues/detail?id=47125 for further discussion.

• bindings/v8/DOMData.h: (WebCore::DOMData::handleWeakObject):
• bindings/v8/DOMDataStore.cpp: (WebCore::DOMDataStore::weakNodeCallback):
• bindings/v8/V8DOMMap.h: (WebCore::WeakReferenceMap::~WeakReferenceMap):

Location:

trunk/WebCore

Files:

• ChangeLog (modified) (1 diff)
• bindings/v8/DOMData.h (modified) (2 diffs)
• bindings/v8/DOMDataStore.cpp (modified) (1 diff)
• bindings/v8/V8DOMMap.h (modified) (2 diffs)

trunk/WebCore/ChangeLog

@@ +1 @@
+2010-07-20  Anton Muhin  <antonm@chromium.org>
+
+        Reviewed by Adam Barth.
+
+        [v8] Allow handles to be disposed and WebKit objects to be dereferenced even if their map is already destroyed.
+        https://bugs.webkit.org/show_bug.cgi?id=42634
+
+        Currently DOMDataStore could be destroyed even if it has some mappings (it gets destroyed
+        when its isolated context gets GCed).  However in this case, handles allocated for
+        such objects would never be disposed as we require presence of mapping from wrapped
+        WebKit object to handle being collected in the map and now map is gone.  That leads to
+        zombie objects in both WebKit (wrapped WebKit object doesn't get dereferenced) and V8
+        (both handle and V8 wrapper object could not be destroyed).
+
+        See http://code.google.com/p/chromium/issues/detail?id=47125 for further discussion.
+
+        * bindings/v8/DOMData.h:
+        (WebCore::DOMData::handleWeakObject):
+        * bindings/v8/DOMDataStore.cpp:
+        (WebCore::DOMDataStore::weakNodeCallback):
+        * bindings/v8/V8DOMMap.h:
+        (WebCore::WeakReferenceMap::~WeakReferenceMap):
+
 2010-07-20  Hans Wennborg  <hans@chromium.org>
 

trunk/WebCore/bindings/v8/DOMData.h

@@ -80 +80 @@
     void DOMData::handleWeakObject(DOMDataStore::DOMWrapperMapType mapType, v8::Persistent<v8::Object> v8Object, T* domObject)
     {
+        WrapperTypeInfo* type = V8DOMWrapper::domWrapperType(v8Object);
         DOMDataList& list = DOMDataStore::allStores();
+        bool found = false;
         for (size_t i = 0; i < list.size(); ++i) {
             DOMDataStore* store = list[i];
@@ -86 +88 @@
 
             DOMWrapperMap<T>* domMap = static_cast<DOMWrapperMap<T>*>(store->getDOMWrapperMap(mapType));
-            if (domMap->removeIfPresent(domObject, v8Object))
-                store->domData()->derefObject(V8DOMWrapper::domWrapperType(v8Object), domObject);
+            if (domMap->removeIfPresent(domObject, v8Object)) {
+                derefObject(type, domObject);
+                found = true;
+            }
+        }
+
+        // If not found, it means map for the wrapper has been already destroyed, just dispose the
+        // handle and deref the object to fight memory leak.
+        if (!found) {
+            v8Object.Dispose();
+            derefObject(type, domObject);
         }
     }

trunk/WebCore/bindings/v8/DOMDataStore.cpp

@@ -164 +164 @@
         if (store->domNodeMap().removeIfPresent(node, v8Object)) {
             ASSERT(store->domData()->owningThread() == WTF::currentThread());
-            node->deref();  // Nobody overrides Node::deref so it's safe
-            break;  // There might be at most one wrapper for the node in world's maps
+            node->deref(); // Nobody overrides Node::deref so it's safe
+            return; // There might be at most one wrapper for the node in world's maps
         }
     }
+
+    // If not found, it means map for the wrapper has been already destroyed, just dispose the
+    // handle and deref the object to fight memory leak.
+    v8Object.Dispose();
+    node->deref(); // Nobody overrides Node::deref so it's safe
 }
 

trunk/WebCore/bindings/v8/V8DOMMap.h

@@ -75 +75 @@
         typedef AbstractWeakReferenceMap<KeyType, ValueType> Parent;
         WeakReferenceMap(v8::WeakReferenceCallback callback) : Parent(callback) { }
-        virtual ~WeakReferenceMap()
-        {
-    #ifndef NDEBUG
-            if (m_map.size() > 0)
-                fprintf(stderr, "Leak %d JS wrappers.\n", m_map.size());
-    #endif
-        }
+        virtual ~WeakReferenceMap() { }
 
         // Get the JS wrapper object of an object.
@@ -136 +130 @@
     protected:
         HashMap<KeyType*, ValueType*> m_map;
-        v8::WeakReferenceCallback m_weakReferenceCallback;
     };