2011-04-15 Geoffrey Garen <ggaren@apple.com>
authorggaren@apple.com <ggaren@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 15 Apr 2011 23:38:04 +0000 (23:38 +0000)
committerggaren@apple.com <ggaren@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 15 Apr 2011 23:38:04 +0000 (23:38 +0000)
        Reviewed by Oliver Hunt.

        DOM object handles are never removed from cache
        https://bugs.webkit.org/show_bug.cgi?id=58707

        We were trying to remove hash table items by value instead of by key.

        * bindings/js/DOMWrapperWorld.cpp:
        (WebCore::JSNodeHandleOwner::finalize): Changed to work more like
        DOMObjectHandleOwner::finalize because I'm going to merge them.

        (WebCore::DOMObjectHandleOwner::finalize): Remove hash table items
        by key, not value. (Oops!) Use a helper function to make sure we get
        this right.

        * bindings/js/JSDOMBinding.cpp:
        (WebCore::cacheDOMObjectWrapper): Store the hash table key as our weak
        handle context, so we can use it at destruction time.

        * bindings/js/JSDOMBinding.h: Removed unnecessary include.

        * bindings/js/JSNodeCustom.h:
        (WebCore::cacheDOMNodeWrapper): Store the hash table key as our weak
        handle context, so we can use it at destruction time.

        * bindings/js/ScriptWrappable.h:
        (WebCore::ScriptWrappable::setWrapper): Forward context parameter, to
        support the above.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@84049 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog
Source/WebCore/bindings/js/DOMWrapperWorld.cpp
Source/WebCore/bindings/js/JSDOMBinding.cpp
Source/WebCore/bindings/js/JSDOMBinding.h
Source/WebCore/bindings/js/JSNodeCustom.h
Source/WebCore/bindings/js/ScriptWrappable.h

index 1b12c3d..a59d569 100644 (file)
@@ -1,3 +1,34 @@
+2011-04-15  Geoffrey Garen  <ggaren@apple.com>
+
+        Reviewed by Oliver Hunt.
+
+        DOM object handles are never removed from cache
+        https://bugs.webkit.org/show_bug.cgi?id=58707
+
+        We were trying to remove hash table items by value instead of by key.
+
+        * bindings/js/DOMWrapperWorld.cpp:
+        (WebCore::JSNodeHandleOwner::finalize): Changed to work more like
+        DOMObjectHandleOwner::finalize because I'm going to merge them.
+
+        (WebCore::DOMObjectHandleOwner::finalize): Remove hash table items
+        by key, not value. (Oops!) Use a helper function to make sure we get
+        this right.
+
+        * bindings/js/JSDOMBinding.cpp:
+        (WebCore::cacheDOMObjectWrapper): Store the hash table key as our weak
+        handle context, so we can use it at destruction time.
+
+        * bindings/js/JSDOMBinding.h: Removed unnecessary include.
+
+        * bindings/js/JSNodeCustom.h:
+        (WebCore::cacheDOMNodeWrapper): Store the hash table key as our weak
+        handle context, so we can use it at destruction time.
+
+        * bindings/js/ScriptWrappable.h:
+        (WebCore::ScriptWrappable::setWrapper): Forward context parameter, to
+        support the above.
+
 2011-04-15  Kenneth Russell  <kbr@google.com>
 
         Unreviewed. Chromium Linux Release build fix due to unused variables.
index 95030db..54ddf54 100644 (file)
@@ -188,10 +188,10 @@ bool JSNodeHandleOwner::isReachableFromOpaqueRoots(JSC::Handle<JSC::Unknown> han
     return isReachableFromDOM(jsNode, jsNode->impl(), m_world, markStack);
 }
 
-void JSNodeHandleOwner::finalize(JSC::Handle<JSC::Unknown> handle, void*)
+void JSNodeHandleOwner::finalize(JSC::Handle<JSC::Unknown> handle, void* context)
 {
     JSNode* jsNode = static_cast<JSNode*>(handle.get().asCell());
-    uncacheDOMNodeWrapper(m_world, jsNode->impl(), jsNode);
+    uncacheDOMNodeWrapper(m_world, static_cast<Node*>(context), jsNode);
 }
 
 bool DOMObjectHandleOwner::isReachableFromOpaqueRoots(JSC::Handle<JSC::Unknown>, void*, JSC::MarkStack&)
@@ -199,10 +199,10 @@ bool DOMObjectHandleOwner::isReachableFromOpaqueRoots(JSC::Handle<JSC::Unknown>,
     return false;
 }
 
-void DOMObjectHandleOwner::finalize(JSC::Handle<JSC::Unknown> handle, void*)
+void DOMObjectHandleOwner::finalize(JSC::Handle<JSC::Unknown> handle, void* context)
 {
     DOMObject* domObject = static_cast<DOMObject*>(handle.get().asCell());
-    m_world->m_wrappers.remove(domObject);
+    uncacheDOMObjectWrapper(m_world, context, domObject);
 }
 
 } // namespace WebCore
index c46d34c..3f1b69d 100644 (file)
@@ -142,12 +142,12 @@ DOMObject* getCachedDOMObjectWrapper(DOMWrapperWorld* world, void* objectHandle)
 
 void cacheDOMObjectWrapper(DOMWrapperWorld* world, void* objectHandle, DOMObject* wrapper) 
 {
-    world->m_wrappers.set(objectHandle, Weak<DOMObject>(*world->globalData(), wrapper, world->domObjectHandleOwner()));
+    world->m_wrappers.set(objectHandle, Weak<DOMObject>(*world->globalData(), wrapper, world->domObjectHandleOwner(), objectHandle));
 }
 
 void uncacheDOMObjectWrapper(DOMWrapperWorld* world, void* objectHandle, DOMObject* wrapper)
 {
-    ASSERT_UNUSED(wrapper, world->m_wrappers.get(objectHandle) == wrapper);
+    ASSERT_UNUSED(wrapper, world->m_wrappers.find(objectHandle)->second.get() == wrapper);
     world->m_wrappers.remove(objectHandle);
 }
 
index fa8f7da..ee541a2 100644 (file)
@@ -28,7 +28,6 @@
 #include "Document.h"
 #include <runtime/Completion.h>
 #include <runtime/Lookup.h>
-#include <runtime/WeakGCMap.h>
 #include <wtf/Forward.h>
 #include <wtf/Noncopyable.h>
 
index 04d0492..dbbb009 100644 (file)
@@ -42,7 +42,7 @@ inline void cacheDOMNodeWrapper(DOMWrapperWorld* world, Node* node, JSNode* wrap
 {
     ASSERT(wrapper);
     if (world->isNormal()) {
-        node->setWrapper(*world->globalData(), wrapper, world->jsNodeHandleOwner());
+        node->setWrapper(*world->globalData(), wrapper, world->jsNodeHandleOwner(), node);
         return;
     }
     cacheDOMObjectWrapper(world, node, wrapper);
index 1111b55..9ff57ca 100644 (file)
@@ -43,9 +43,9 @@ public:
         return m_wrapper.get();
     }
 
-    void setWrapper(JSC::JSGlobalData& globalData, DOMObject* wrapper, JSC::WeakHandleOwner* wrapperOwner)
+    void setWrapper(JSC::JSGlobalData& globalData, DOMObject* wrapper, JSC::WeakHandleOwner* wrapperOwner, void* context)
     {
-        m_wrapper.set(globalData, wrapper, wrapperOwner);
+        m_wrapper.set(globalData, wrapper, wrapperOwner, context);
     }
 
     void clearWrapper()