2011-01-28 Michael Saboff <msaboff@apple.com>
authormsaboff@apple.com <msaboff@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 28 Jan 2011 20:21:07 +0000 (20:21 +0000)
committermsaboff@apple.com <msaboff@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 28 Jan 2011 20:21:07 +0000 (20:21 +0000)
        Potentially Unsafe HashSet of RuntimeObject* in RootObject definition
        https://bugs.webkit.org/show_bug.cgi?id=53271

        Reapplying this this change.  No change from prior patch in
        JavaScriptCore.

        Added new isValid() methods to check if a contained object in
        a WeakGCMap is valid when using an unchecked iterator.

        * runtime/WeakGCMap.h:
        (JSC::WeakGCMap::isValid):
2011-01-28  Michael Saboff  <msaboff@apple.com>

        Reviewed by Geoffrey Garen.

        Potentially Unsafe HashSet of RuntimeObject* in RootObject definition
        https://bugs.webkit.org/show_bug.cgi?id=53271

        Reapplying this patch with the change that the second ASSERT in
        RootObject::removeRuntimeObject was changed to use
        .uncheckedGet() instead of the failing .get().  The object in question
        could be in the process of being GC'ed.  The get() call will not return
        such an object while the uncheckedGet() call will return the (unsafe)
        object.  This is the behavior we want.

        Precautionary change.
        Changed RootObject to use WeakGCMap instead of HashSet.
        Found will looking for another issue, but can't produce a test case
        that is problematic.  THerefore there aren't any new tests.

        * bridge/runtime_root.cpp:
        (JSC::Bindings::RootObject::invalidate):
        (JSC::Bindings::RootObject::addRuntimeObject):
        (JSC::Bindings::RootObject::removeRuntimeObject):
        * bridge/runtime_root.h:

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

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/runtime/WeakGCMap.h
Source/WebCore/ChangeLog
Source/WebCore/bridge/runtime_root.cpp
Source/WebCore/bridge/runtime_root.h

index bde0be9..2ff2338 100644 (file)
@@ -1,3 +1,17 @@
+2011-01-28  Michael Saboff  <msaboff@apple.com>
+
+        Potentially Unsafe HashSet of RuntimeObject* in RootObject definition
+        https://bugs.webkit.org/show_bug.cgi?id=53271
+
+        Reapplying this this change.  No change from prior patch in
+        JavaScriptCore.
+
+        Added new isValid() methods to check if a contained object in
+        a WeakGCMap is valid when using an unchecked iterator.
+
+        * runtime/WeakGCMap.h:
+        (JSC::WeakGCMap::isValid):
+
 2011-01-27  Adam Roben  <aroben@apple.com>
 
         Extract code to convert a WTF absolute time to a Win32 wait interval into a separate
index 316794f..c063dd2 100644 (file)
@@ -69,6 +69,9 @@ public:
     const_iterator uncheckedBegin() const { return m_map.begin(); }
     const_iterator uncheckedEnd() const { return m_map.end(); }
 
+    bool isValid(iterator it) const { return Heap::isCellMarked(it->second); }
+    bool isValid(const_iterator it) const { return Heap::isCellMarked(it->second); }
+
 private:
     HashMap<KeyType, MappedType> m_map;
 };
index f0bcb98..6a60150 100644 (file)
@@ -1,3 +1,28 @@
+2011-01-28  Michael Saboff  <msaboff@apple.com>
+
+        Reviewed by Geoffrey Garen.
+
+        Potentially Unsafe HashSet of RuntimeObject* in RootObject definition
+        https://bugs.webkit.org/show_bug.cgi?id=53271
+
+        Reapplying this patch with the change that the second ASSERT in 
+        RootObject::removeRuntimeObject was changed to use
+        .uncheckedGet() instead of the failing .get().  The object in question
+        could be in the process of being GC'ed.  The get() call will not return
+        such an object while the uncheckedGet() call will return the (unsafe) 
+        object.  This is the behavior we want.
+
+        Precautionary change.
+        Changed RootObject to use WeakGCMap instead of HashSet.
+        Found will looking for another issue, but can't produce a test case
+        that is problematic.  THerefore there aren't any new tests.
+
+        * bridge/runtime_root.cpp:
+        (JSC::Bindings::RootObject::invalidate):
+        (JSC::Bindings::RootObject::addRuntimeObject):
+        (JSC::Bindings::RootObject::removeRuntimeObject):
+        * bridge/runtime_root.h:
+
 2011-01-28  Adam Roben  <aroben@apple.com>
 
         Notify CACFLayerTreeHost when the context is flushed
index 796354f..3c8b313 100644 (file)
@@ -101,13 +101,15 @@ void RootObject::invalidate()
         return;
 
     {
-        HashSet<RuntimeObject*>::iterator end = m_runtimeObjects.end();
-        for (HashSet<RuntimeObject*>::iterator it = m_runtimeObjects.begin(); it != end; ++it)
-            (*it)->invalidate();
-        
+        WeakGCMap<RuntimeObject*, RuntimeObject*>::iterator end = m_runtimeObjects.uncheckedEnd();
+        for (WeakGCMap<RuntimeObject*, RuntimeObject*>::iterator it = m_runtimeObjects.uncheckedBegin(); it != end; ++it) {
+            if (m_runtimeObjects.isValid(it))
+                it->second->invalidate();
+        }
+
         m_runtimeObjects.clear();
     }
-    
+
     m_isValid = false;
 
     m_nativeHandle = 0;
@@ -176,17 +178,17 @@ void RootObject::updateGlobalObject(JSGlobalObject* globalObject)
 void RootObject::addRuntimeObject(RuntimeObject* object)
 {
     ASSERT(m_isValid);
-    ASSERT(!m_runtimeObjects.contains(object));
-    
-    m_runtimeObjects.add(object);
-}        
-    
+    ASSERT(!m_runtimeObjects.get(object));
+
+    m_runtimeObjects.set(object, object);
+}
+
 void RootObject::removeRuntimeObject(RuntimeObject* object)
 {
     ASSERT(m_isValid);
-    ASSERT(m_runtimeObjects.contains(object));
-    
-    m_runtimeObjects.remove(object);
+    ASSERT(m_runtimeObjects.uncheckedGet(object));
+
+    m_runtimeObjects.take(object);
 }
 
 } } // namespace JSC::Bindings
index babd7ad..8290e7c 100644 (file)
@@ -31,8 +31,8 @@
 #endif
 #include <runtime/Protect.h>
 
+#include <runtime/WeakGCMap.h>
 #include <wtf/Forward.h>
-#include <wtf/HashSet.h>
 #include <wtf/Noncopyable.h>
 #include <wtf/PassRefPtr.h>
 #include <wtf/RefCounted.h>
@@ -89,7 +89,7 @@ private:
     ProtectedPtr<JSGlobalObject> m_globalObject;
 
     ProtectCountSet m_protectCountSet;
-    HashSet<RuntimeObject*> m_runtimeObjects;    
+    WeakGCMap<RuntimeObject*, RuntimeObject*> m_runtimeObjects; // Really need a WeakGCSet, but this will do.
 
     HashSet<InvalidationCallback*> m_invalidationCallbacks;
 };