Only allow non-null pointers in the WeakSet

https://bugs.webkit.org/show_bug.cgi?id=85119

Reviewed by Darin Adler.

../JavaScriptCore:

This is a step toward more efficient finalization.

No clients put non-pointers (JSValues) into Weak<T> and PassWeak<T>.

Some clients put null pointers into Weak<T> and PassWeak<T>, but this is
more efficient and straight-forward to model with a null in the Weak<T>
or PassWeak<T> instead of allocating a WeakImpl just to hold null.

* heap/PassWeak.h:
(JSC): Removed the Unknown (JSValue) type of weak pointer because it's
unused now.

(PassWeak): Don't provide a default initializer for our JSCell* argument.
This feature was only used in one place, and it was a bug.

(JSC::::get): Don't check for a null stored inside our WeakImpl: that's
not allowed anymore.

(JSC::PassWeak::PassWeak): Handle null as a null WeakImpl instead of
allocating a WeakImpl and storing null into it.

* heap/Weak.h:
(Weak):
(JSC::::Weak): Same changes as in PassWeak<T>.

* heap/WeakBlock.cpp:
(JSC::WeakBlock::visitLiveWeakImpls):
(JSC::WeakBlock::visitDeadWeakImpls): Only non-null cells are valid in
the WeakSet now, so no need to check for non-cells and null cell pointers.

* heap/WeakImpl.h:
(JSC::WeakImpl::WeakImpl): Only non-null cells are valid in the WeakSet
now, so ASSERT that.

../WebCore:

* bridge/jsc/BridgeJSC.cpp:
(JSC::Bindings::Instance::Instance): Don't allocate a WeakImpl just to
store null. This was needless, and is now a compile error. Instead,
rely on the default constructor, which will produce a cheap null.

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

diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
index df935de..c2eb9b1 100644 (file)
--- a/Source/JavaScriptCore/ChangeLog
+++ b/Source/JavaScriptCore/ChangeLog
@@ -1,3 +1,44 @@
+2012-04-27  Geoffrey Garen  <ggaren@apple.com>
+
+        Only allow non-null pointers in the WeakSet
+        https://bugs.webkit.org/show_bug.cgi?id=85119
+
+        Reviewed by Darin Adler.
+
+        This is a step toward more efficient finalization.
+
+        No clients put non-pointers (JSValues) into Weak<T> and PassWeak<T>.
+
+        Some clients put null pointers into Weak<T> and PassWeak<T>, but this is
+        more efficient and straight-forward to model with a null in the Weak<T>
+        or PassWeak<T> instead of allocating a WeakImpl just to hold null.
+
+        * heap/PassWeak.h:
+        (JSC): Removed the Unknown (JSValue) type of weak pointer because it's
+        unused now.
+
+        (PassWeak): Don't provide a default initializer for our JSCell* argument.
+        This feature was only used in one place, and it was a bug.
+
+        (JSC::::get): Don't check for a null stored inside our WeakImpl: that's 
+        not allowed anymore.
+
+        (JSC::PassWeak::PassWeak): Handle null as a null WeakImpl instead of
+        allocating a WeakImpl and storing null into it.
+
+        * heap/Weak.h:
+        (Weak):
+        (JSC::::Weak): Same changes as in PassWeak<T>.
+
+        * heap/WeakBlock.cpp:
+        (JSC::WeakBlock::visitLiveWeakImpls):
+        (JSC::WeakBlock::visitDeadWeakImpls): Only non-null cells are valid in
+        the WeakSet now, so no need to check for non-cells and null cell pointers.
+
+        * heap/WeakImpl.h:
+        (JSC::WeakImpl::WeakImpl): Only non-null cells are valid in the WeakSet
+        now, so ASSERT that.
+
 2012-04-27  Gavin Barraclough  <barraclough@apple.com>
 
         <rdar://problem/7909395> Math in JavaScript is inaccurate on iOS

diff --git a/Source/JavaScriptCore/heap/PassWeak.h b/Source/JavaScriptCore/heap/PassWeak.h
index d6f4db2..8a55737 100644 (file)
--- a/Source/JavaScriptCore/heap/PassWeak.h
+++ b/Source/JavaScriptCore/heap/PassWeak.h
@@ -50,15 +50,6 @@ public:
 #endif
 };
 
-template<typename Base> class WeakImplAccessor<Base, Unknown> {
-public:
-    typedef JSValue GetType;
-
-    const JSValue* operator->() const;
-    const JSValue& operator*() const;
-    GetType get() const;
-};
-
 template<typename T> class PassWeak : public WeakImplAccessor<PassWeak<T>, T> {
 public:
     friend class WeakImplAccessor<PassWeak<T>, T>;
@@ -66,7 +57,7 @@ public:
 
     PassWeak();
     PassWeak(std::nullptr_t);
-    PassWeak(JSGlobalData&, GetType = GetType(), WeakHandleOwner* = 0, void* context = 0);
+    PassWeak(JSGlobalData&, GetType, WeakHandleOwner* = 0, void* context = 0);
 
     // It somewhat breaks the type system to allow transfer of ownership out of
     // a const PassWeak. However, it makes it much easier to work with PassWeak
@@ -105,7 +96,7 @@ template<typename Base, typename T> inline T& WeakImplAccessor<Base, T>::operato
 
 template<typename Base, typename T> inline typename WeakImplAccessor<Base, T>::GetType WeakImplAccessor<Base, T>::get() const
 {
-    if (!static_cast<const Base*>(this)->m_impl || static_cast<const Base*>(this)->m_impl->state() != WeakImpl::Live || !static_cast<const Base*>(this)->m_impl->jsValue())
+    if (!static_cast<const Base*>(this)->m_impl || static_cast<const Base*>(this)->m_impl->state() != WeakImpl::Live)
         return GetType();
     return jsCast<T*>(static_cast<const Base*>(this)->m_impl->jsValue().asCell());
 }
@@ -117,25 +108,6 @@ template<typename Base, typename T> inline bool WeakImplAccessor<Base, T>::was(t
 }
 #endif
 
-template<typename Base> inline const JSValue* WeakImplAccessor<Base, Unknown>::operator->() const
-{
-    ASSERT(static_cast<const Base*>(this)->m_impl && static_cast<const Base*>(this)->m_impl->state() == WeakImpl::Live);
-    return &static_cast<const Base*>(this)->m_impl->jsValue();
-}
-
-template<typename Base> inline const JSValue& WeakImplAccessor<Base, Unknown>::operator*() const
-{
-    ASSERT(static_cast<const Base*>(this)->m_impl && static_cast<const Base*>(this)->m_impl->state() == WeakImpl::Live);
-    return static_cast<const Base*>(this)->m_impl->jsValue();
-}
-
-template<typename Base> inline typename WeakImplAccessor<Base, Unknown>::GetType WeakImplAccessor<Base, Unknown>::get() const
-{
-    if (!static_cast<const Base*>(this)->m_impl || static_cast<const Base*>(this)->m_impl->state() != WeakImpl::Live)
-        return GetType();
-    return static_cast<const Base*>(this)->m_impl->jsValue();
-}
-
 template<typename T> inline PassWeak<T>::PassWeak()
     : m_impl(0)
 {
@@ -147,7 +119,7 @@ template<typename T> inline PassWeak<T>::PassWeak(std::nullptr_t)
 }
 
 template<typename T> inline PassWeak<T>::PassWeak(JSGlobalData& globalData, typename PassWeak<T>::GetType getType, WeakHandleOwner* weakOwner, void* context)
-    : m_impl(globalData.heap.weakSet()->allocate(getType, weakOwner, context))
+    : m_impl(getType ? globalData.heap.weakSet()->allocate(getType, weakOwner, context) : 0)
 {
 }
 

diff --git a/Source/JavaScriptCore/heap/Weak.h b/Source/JavaScriptCore/heap/Weak.h
index b7f85ab..e99276e 100644 (file)
--- a/Source/JavaScriptCore/heap/Weak.h
+++ b/Source/JavaScriptCore/heap/Weak.h
@@ -40,7 +40,7 @@ public:
 
     Weak();
     Weak(std::nullptr_t);
-    Weak(JSGlobalData&, GetType = GetType(), WeakHandleOwner* = 0, void* context = 0);
+    Weak(JSGlobalData&, GetType, WeakHandleOwner* = 0, void* context = 0);
 
     enum HashTableDeletedValueTag { HashTableDeletedValue };
     bool isHashTableDeletedValue() const;
@@ -79,7 +79,7 @@ template<typename T> inline Weak<T>::Weak(std::nullptr_t)
 }
 
 template<typename T> inline Weak<T>::Weak(JSGlobalData& globalData, typename Weak<T>::GetType getType, WeakHandleOwner* weakOwner, void* context)
-    : m_impl(globalData.heap.weakSet()->allocate(getType, weakOwner, context))
+    : m_impl(getType ? globalData.heap.weakSet()->allocate(getType, weakOwner, context) : 0)
 {
 }
 

diff --git a/Source/JavaScriptCore/heap/WeakBlock.cpp b/Source/JavaScriptCore/heap/WeakBlock.cpp
index f144f28..13c20ae 100644 (file)
--- a/Source/JavaScriptCore/heap/WeakBlock.cpp
+++ b/Source/JavaScriptCore/heap/WeakBlock.cpp
@@ -104,11 +104,7 @@ void WeakBlock::visitLiveWeakImpls(HeapRootVisitor& heapRootVisitor)
             continue;
 
         const JSValue& jsValue = weakImpl->jsValue();
-        if (!jsValue || !jsValue.isCell())
-            continue;
-
-        JSCell* jsCell = jsValue.asCell();
-        if (Heap::isMarked(jsCell))
+        if (Heap::isMarked(jsValue.asCell()))
             continue;
 
         WeakHandleOwner* weakHandleOwner = weakImpl->weakHandleOwner();
@@ -133,12 +129,7 @@ void WeakBlock::visitDeadWeakImpls(HeapRootVisitor&)
         if (weakImpl->state() > WeakImpl::Dead)
             continue;
 
-        const JSValue& jsValue = weakImpl->jsValue();
-        if (!jsValue || !jsValue.isCell())
-            continue;
-
-        JSCell* jsCell = jsValue.asCell();
-        if (Heap::isMarked(jsCell))
+        if (Heap::isMarked(weakImpl->jsValue().asCell()))
             continue;
 
         weakImpl->setState(WeakImpl::Dead);

diff --git a/Source/JavaScriptCore/heap/WeakImpl.h b/Source/JavaScriptCore/heap/WeakImpl.h
index 5467761..9924923 100644 (file)
--- a/Source/JavaScriptCore/heap/WeakImpl.h
+++ b/Source/JavaScriptCore/heap/WeakImpl.h
@@ -76,6 +76,7 @@ inline WeakImpl::WeakImpl(JSValue jsValue, WeakHandleOwner* weakHandleOwner, voi
     , m_context(context)
 {
     ASSERT(state() == Live);
+    ASSERT(m_jsValue && m_jsValue.isCell());
 }
 
 inline WeakImpl::State WeakImpl::state()

diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
index e5a9918..349a9e4 100644 (file)
--- a/Source/WebCore/ChangeLog
+++ b/Source/WebCore/ChangeLog
@@ -1,3 +1,15 @@
+2012-04-27  Geoffrey Garen  <ggaren@apple.com>
+
+        Only allow non-null pointers in the WeakSet
+        https://bugs.webkit.org/show_bug.cgi?id=85119
+
+        Reviewed by Darin Adler.
+
+        * bridge/jsc/BridgeJSC.cpp:
+        (JSC::Bindings::Instance::Instance): Don't allocate a WeakImpl just to
+        store null. This was needless, and is now a compile error. Instead,
+        rely on the default constructor, which will produce a cheap null.
+
 2012-04-27  Kentaro Hara  <haraken@chromium.org>
 
         "Not enough arguments" error should be TypeError

diff --git a/Source/WebCore/bridge/jsc/BridgeJSC.cpp b/Source/WebCore/bridge/jsc/BridgeJSC.cpp
index 3bcb29b..115b07c 100644 (file)
--- a/Source/WebCore/bridge/jsc/BridgeJSC.cpp
+++ b/Source/WebCore/bridge/jsc/BridgeJSC.cpp
@@ -54,7 +54,6 @@ Array::~Array()
 
 Instance::Instance(PassRefPtr<RootObject> rootObject)
     : m_rootObject(rootObject)
-    , m_runtimeObject(*WebCore::JSDOMWindowBase::commonJSGlobalData())
 {
     ASSERT(m_rootObject);
 }