Skip to content
Permalink
Browse files
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):

Canonical link: https://commits.webkit.org/54589@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@63751 268f45cc-cd09-0410-ab3c-d52691b4dbfc
  • Loading branch information
antonm committed Jul 20, 2010
1 parent c37966a commit 6f1c1bfa9cd83d09b55b19acc66aa8a2d1f2e7f0
Showing with 44 additions and 12 deletions.
  1. +23 −0 WebCore/ChangeLog
  2. +13 −2 WebCore/bindings/v8/DOMData.h
  3. +7 −2 WebCore/bindings/v8/DOMDataStore.cpp
  4. +1 −8 WebCore/bindings/v8/V8DOMMap.h
@@ -1,3 +1,26 @@
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>

Reviewed by Steve Block.
@@ -79,14 +79,25 @@ namespace WebCore {
template<typename T>
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];
ASSERT(store->domData()->owningThread() == WTF::currentThread());

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);
}
}

@@ -163,10 +163,15 @@ void DOMDataStore::weakNodeCallback(v8::Persistent<v8::Value> v8Object, void* do
DOMDataStore* store = list[i];
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
}

bool DOMDataStore::IntrusiveDOMWrapperMap::removeIfPresent(Node* obj, v8::Persistent<v8::Data> value)
@@ -74,13 +74,7 @@ namespace WebCore {
public:
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.
virtual v8::Persistent<ValueType> get(KeyType* obj)
@@ -135,7 +129,6 @@ namespace WebCore {

protected:
HashMap<KeyType*, ValueType*> m_map;
v8::WeakReferenceCallback m_weakReferenceCallback;
};

template <class KeyType> class DOMWrapperMap : public WeakReferenceMap<KeyType, v8::Object> {

0 comments on commit 6f1c1bf

Please sign in to comment.