Changeset 76969 in webkit

Timestamp:

Jan 28, 2011 12:21:07 PM (13 years ago)

Author:

msaboff@apple.com

Message:

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-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:

Location:

trunk/Source

Files:

• JavaScriptCore/ChangeLog (modified) (1 diff)
• JavaScriptCore/runtime/WeakGCMap.h (modified) (1 diff)
• WebCore/ChangeLog (modified) (1 diff)
• WebCore/bridge/runtime_root.cpp (modified) (2 diffs)
• WebCore/bridge/runtime_root.h (modified) (2 diffs)

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/JavaScriptCore/runtime/WeakGCMap.h

@@ -70 +70 @@
     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;

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/WebCore/bridge/runtime_root.cpp

@@ -102 +102 @@
 
     {
-        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;
 
@@ -177 +179 @@
 {
     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);
 }
 

trunk/Source/WebCore/bridge/runtime_root.h

@@ -32 +32 @@
 #include <runtime/Protect.h>
 
+#include <runtime/WeakGCMap.h>
 #include <wtf/Forward.h>
-#include <wtf/HashSet.h>
 #include <wtf/Noncopyable.h>
 #include <wtf/PassRefPtr.h>
@@ -90 +90 @@
 
     ProtectCountSet m_protectCountSet;
-    HashSet<RuntimeObject*> m_runtimeObjects;    
+    WeakGCMap<RuntimeObject*, RuntimeObject*> m_runtimeObjects; // Really need a WeakGCSet, but this will do.
 
     HashSet<InvalidationCallback*> m_invalidationCallbacks;