Changeset 165946 in webkit for trunk/Source/JavaScriptCore/runtime/PropertyName.h

Timestamp:

Mar 19, 2014, 11:21:49 PM (11 years ago)

Author:

[email protected]

Message:

​https://bugs.webkit.org/show_bug.cgi?id=130494
EmptyUnique strings are Identifiers/Atomic

Reviewed by Geoff Garen.

EmptyUnique strings should set the Identifier/Atomic flag.

Source/JavaScriptCore:

This fixes an unreproducible bug we believe exists in Identifier handling.
Expected behaviour is that while Identifiers may reference EmptyUniques
(StringImpls allocated as UIDs for PrivateNames), these are not created
through the main Identifier constructor, the Identifier flag is not set
on PrivateNames, and we should never lookup EmptyUnique strings in the
IdentifierTable.

Unfortunately that was happening. Some tables used to implement property
access in the JIT hold StringImpl*s, and turn these back into Identifiers
using the identfiier constructor. Since the code generator will now plant
by-id (cachable) accesses to PrivateNames we can end up passing an
EmptyUnique to Identifier::add, potentially leading to PrivateNames being
uniqued together (though hard to prove, since the hash codes are random).

• runtime/PropertyName.h:

(JSC::PropertyName::PropertyName):
(JSC::PropertyName::uid):
(JSC::PropertyName::publicName):
(JSC::PropertyName::asIndex):

• PropertyName assumed that PrivateNames are not Identifiers - instead check isEmptyUnique().

• runtime/Structure.cpp:

(JSC::Structure::getPropertyNamesFromStructure):

• Structure assumed that PrivateNames are not Identifiers - instead check isEmptyUnique().

Source/WTF:

• wtf/text/AtomicString.h:

(WTF::AtomicString::add):

• Previously we assumed the only StringImpl that was validly allowed to claim to be Atomic but not be in a table was the canonical empty string. Now that EmptyUniques are also marked Atomic, all empty strings may pass this condition.

• wtf/text/StringImpl.cpp:

(WTF::StringImpl::~StringImpl):

• EmptyUnique strings are not in the Atomic/Identfiier tabels, so don't need removing.

• wtf/text/StringImpl.h:

(WTF::StringImpl::StringImpl):

• Change EmptyUnique constructor to call hashAndFlagsForEmptyUnique.

• wtf/text/StringStatics.cpp:

(WTF::StringImpl::hashAndFlagsForEmptyUnique):

• Allocate a sequential hash code (this should be just as good for distribution & better for debugging than the random value) and set flags, now including Atomic & Identifier.

File:

• trunk/Source/JavaScriptCore/runtime/PropertyName.h (modified) (4 diffs)

trunk/Source/JavaScriptCore/runtime/PropertyName.h

@@ -82 +82 @@
         : m_impl(propertyName.impl())
     {
-        ASSERT(!m_impl || m_impl->isIdentifier() || m_impl->isEmptyUnique());
+        ASSERT(!m_impl || m_impl->isIdentifier());
     }
 
@@ -93 +93 @@
     StringImpl* uid() const
     {
-        ASSERT(!m_impl || (m_impl->isIdentifier() == !m_impl->isEmptyUnique()));
         return m_impl;
     }
@@ -99 +98 @@
     StringImpl* publicName() const
     {
-        ASSERT(!m_impl || (m_impl->isIdentifier() == !m_impl->isEmptyUnique()));
-        return m_impl->isIdentifier() ? m_impl : 0;
+        return m_impl->isEmptyUnique() ? 0 : m_impl;
     }
 
@@ -107 +105 @@
     uint32_t asIndex()
     {
-        ASSERT(!m_impl || (m_impl->isIdentifier() == !m_impl->isEmptyUnique()));
         return m_impl ? toUInt32FromStringImpl(m_impl) : NotAnIndex;
     }