<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1//EN"
"http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head><meta http-equiv="content-type" content="text/html; charset=utf-8" />
<title>[165946] trunk/Source</title>
</head>
<body>
<style type="text/css"><!--
#msg dl.meta { border: 1px #006 solid; background: #369; padding: 6px; color: #fff; }
#msg dl.meta dt { float: left; width: 6em; font-weight: bold; }
#msg dt:after { content:':';}
#msg dl, #msg dt, #msg ul, #msg li, #header, #footer, #logmsg { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; }
#msg dl a { font-weight: bold}
#msg dl a:link { color:#fc3; }
#msg dl a:active { color:#ff0; }
#msg dl a:visited { color:#cc6; }
h3 { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; font-weight: bold; }
#msg pre { overflow: auto; background: #ffc; border: 1px #fa0 solid; padding: 6px; }
#logmsg { background: #ffc; border: 1px #fa0 solid; padding: 1em 1em 0 1em; }
#logmsg p, #logmsg pre, #logmsg blockquote { margin: 0 0 1em 0; }
#logmsg p, #logmsg li, #logmsg dt, #logmsg dd { line-height: 14pt; }
#logmsg h1, #logmsg h2, #logmsg h3, #logmsg h4, #logmsg h5, #logmsg h6 { margin: .5em 0; }
#logmsg h1:first-child, #logmsg h2:first-child, #logmsg h3:first-child, #logmsg h4:first-child, #logmsg h5:first-child, #logmsg h6:first-child { margin-top: 0; }
#logmsg ul, #logmsg ol { padding: 0; list-style-position: inside; margin: 0 0 0 1em; }
#logmsg ul { text-indent: -1em; padding-left: 1em; }#logmsg ol { text-indent: -1.5em; padding-left: 1.5em; }
#logmsg > ul, #logmsg > ol { margin: 0 0 1em 0; }
#logmsg pre { background: #eee; padding: 1em; }
#logmsg blockquote { border: 1px solid #fa0; border-left-width: 10px; padding: 1em 1em 0 1em; background: white;}
#logmsg dl { margin: 0; }
#logmsg dt { font-weight: bold; }
#logmsg dd { margin: 0; padding: 0 0 0.5em 0; }
#logmsg dd:before { content:'\00bb';}
#logmsg table { border-spacing: 0px; border-collapse: collapse; border-top: 4px solid #fa0; border-bottom: 1px solid #fa0; background: #fff; }
#logmsg table th { text-align: left; font-weight: normal; padding: 0.2em 0.5em; border-top: 1px dotted #fa0; }
#logmsg table td { text-align: right; border-top: 1px dotted #fa0; padding: 0.2em 0.5em; }
#logmsg table thead th { text-align: center; border-bottom: 1px solid #fa0; }
#logmsg table th.Corner { text-align: left; }
#logmsg hr { border: none 0; border-top: 2px dashed #fa0; height: 1px; }
#header, #footer { color: #fff; background: #636; border: 1px #300 solid; padding: 6px; }
#patch { width: 100%; }
#patch h4 {font-family: verdana,arial,helvetica,sans-serif;font-size:10pt;padding:8px;background:#369;color:#fff;margin:0;}
#patch .propset h4, #patch .binary h4 {margin:0;}
#patch pre {padding:0;line-height:1.2em;margin:0;}
#patch .diff {width:100%;background:#eee;padding: 0 0 10px 0;overflow:auto;}
#patch .propset .diff, #patch .binary .diff {padding:10px 0;}
#patch span {display:block;padding:0 10px;}
#patch .modfile, #patch .addfile, #patch .delfile, #patch .propset, #patch .binary, #patch .copfile {border:1px solid #ccc;margin:10px 0;}
#patch ins {background:#dfd;text-decoration:none;display:block;padding:0 10px;}
#patch del {background:#fdd;text-decoration:none;display:block;padding:0 10px;}
#patch .lines, .info {color:#888;background:#fff;}
--></style>
<div id="msg">
<dl class="meta">
<dt>Revision</dt> <dd><a href="http://trac.webkit.org/projects/webkit/changeset/165946">165946</a></dd>
<dt>Author</dt> <dd>barraclough@apple.com</dd>
<dt>Date</dt> <dd>2014-03-19 23:21:49 -0700 (Wed, 19 Mar 2014)</dd>
</dl>
<h3>Log Message</h3>
<pre>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.</pre>
<h3>Modified Paths</h3>
<ul>
<li><a href="#trunkSourceJavaScriptCoreChangeLog">trunk/Source/JavaScriptCore/ChangeLog</a></li>
<li><a href="#trunkSourceJavaScriptCoreruntimePropertyNameh">trunk/Source/JavaScriptCore/runtime/PropertyName.h</a></li>
<li><a href="#trunkSourceJavaScriptCoreruntimeStructurecpp">trunk/Source/JavaScriptCore/runtime/Structure.cpp</a></li>
<li><a href="#trunkSourceWTFChangeLog">trunk/Source/WTF/ChangeLog</a></li>
<li><a href="#trunkSourceWTFwtftextAtomicStringh">trunk/Source/WTF/wtf/text/AtomicString.h</a></li>
<li><a href="#trunkSourceWTFwtftextStringImplcpp">trunk/Source/WTF/wtf/text/StringImpl.cpp</a></li>
<li><a href="#trunkSourceWTFwtftextStringImplh">trunk/Source/WTF/wtf/text/StringImpl.h</a></li>
<li><a href="#trunkSourceWTFwtftextStringStaticscpp">trunk/Source/WTF/wtf/text/StringStatics.cpp</a></li>
</ul>
</div>
<div id="patch">
<h3>Diff</h3>
<a id="trunkSourceJavaScriptCoreChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/ChangeLog (165945 => 165946)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/ChangeLog 2014-03-20 06:17:09 UTC (rev 165945)
+++ trunk/Source/JavaScriptCore/ChangeLog 2014-03-20 06:21:49 UTC (rev 165946)
</span><span class="lines">@@ -1,3 +1,36 @@
</span><ins>+2014-03-19 Gavin Barraclough <barraclough@apple.com>
+
+ 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.
+
+ 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().
+
</ins><span class="cx"> 2014-03-19 Filip Pizlo <fpizlo@apple.com>
</span><span class="cx">
</span><span class="cx"> Unreviewed, revert the DFGCommon.h change in r165938. It was not intentional.
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoreruntimePropertyNameh"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/runtime/PropertyName.h (165945 => 165946)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/runtime/PropertyName.h 2014-03-20 06:17:09 UTC (rev 165945)
+++ trunk/Source/JavaScriptCore/runtime/PropertyName.h 2014-03-20 06:21:49 UTC (rev 165946)
</span><span class="lines">@@ -81,7 +81,7 @@
</span><span class="cx"> PropertyName(const Identifier& propertyName)
</span><span class="cx"> : m_impl(propertyName.impl())
</span><span class="cx"> {
</span><del>- ASSERT(!m_impl || m_impl->isIdentifier() || m_impl->isEmptyUnique());
</del><ins>+ ASSERT(!m_impl || m_impl->isIdentifier());
</ins><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> PropertyName(const PrivateName& propertyName)
</span><span class="lines">@@ -92,21 +92,18 @@
</span><span class="cx">
</span><span class="cx"> StringImpl* uid() const
</span><span class="cx"> {
</span><del>- ASSERT(!m_impl || (m_impl->isIdentifier() == !m_impl->isEmptyUnique()));
</del><span class="cx"> return m_impl;
</span><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> StringImpl* publicName() const
</span><span class="cx"> {
</span><del>- ASSERT(!m_impl || (m_impl->isIdentifier() == !m_impl->isEmptyUnique()));
- return m_impl->isIdentifier() ? m_impl : 0;
</del><ins>+ return m_impl->isEmptyUnique() ? 0 : m_impl;
</ins><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> static const uint32_t NotAnIndex = UINT_MAX;
</span><span class="cx">
</span><span class="cx"> uint32_t asIndex()
</span><span class="cx"> {
</span><del>- ASSERT(!m_impl || (m_impl->isIdentifier() == !m_impl->isEmptyUnique()));
</del><span class="cx"> return m_impl ? toUInt32FromStringImpl(m_impl) : NotAnIndex;
</span><span class="cx"> }
</span><span class="cx">
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoreruntimeStructurecpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/runtime/Structure.cpp (165945 => 165946)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/runtime/Structure.cpp 2014-03-20 06:17:09 UTC (rev 165945)
+++ trunk/Source/JavaScriptCore/runtime/Structure.cpp 2014-03-20 06:21:49 UTC (rev 165946)
</span><span class="lines">@@ -988,7 +988,7 @@
</span><span class="cx"> PropertyTable::iterator end = propertyTable()->end();
</span><span class="cx"> for (PropertyTable::iterator iter = propertyTable()->begin(); iter != end; ++iter) {
</span><span class="cx"> ASSERT(m_hasNonEnumerableProperties || !(iter->attributes & DontEnum));
</span><del>- if (iter->key->isIdentifier() && (!(iter->attributes & DontEnum) || mode == IncludeDontEnumProperties)) {
</del><ins>+ if (!iter->key->isEmptyUnique() && (!(iter->attributes & DontEnum) || mode == IncludeDontEnumProperties)) {
</ins><span class="cx"> if (knownUnique)
</span><span class="cx"> propertyNames.addKnownUnique(iter->key);
</span><span class="cx"> else
</span></span></pre></div>
<a id="trunkSourceWTFChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Source/WTF/ChangeLog (165945 => 165946)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WTF/ChangeLog 2014-03-20 06:17:09 UTC (rev 165945)
+++ trunk/Source/WTF/ChangeLog 2014-03-20 06:21:49 UTC (rev 165946)
</span><span class="lines">@@ -1,5 +1,30 @@
</span><span class="cx"> 2014-03-19 Gavin Barraclough <barraclough@apple.com>
</span><span class="cx">
</span><ins>+ 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.
+
+ * 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.
+
+2014-03-19 Gavin Barraclough <barraclough@apple.com>
+
</ins><span class="cx"> Small cleanup of empty string
</span><span class="cx"> https://bugs.webkit.org/show_bug.cgi?id=130438
</span><span class="cx">
</span></span></pre></div>
<a id="trunkSourceWTFwtftextAtomicStringh"></a>
<div class="modfile"><h4>Modified: trunk/Source/WTF/wtf/text/AtomicString.h (165945 => 165946)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WTF/wtf/text/AtomicString.h 2014-03-20 06:17:09 UTC (rev 165945)
+++ trunk/Source/WTF/wtf/text/AtomicString.h 2014-03-20 06:21:49 UTC (rev 165946)
</span><span class="lines">@@ -185,7 +185,7 @@
</span><span class="cx"> ALWAYS_INLINE static PassRefPtr<StringImpl> add(StringImpl* string)
</span><span class="cx"> {
</span><span class="cx"> if (!string || string->isAtomic()) {
</span><del>- ASSERT_WITH_MESSAGE(!string || string == StringImpl::empty() || isInAtomicStringTable(string), "The atomic string comes from an other thread!");
</del><ins>+ ASSERT_WITH_MESSAGE(!string || !string->length() || isInAtomicStringTable(string), "The atomic string comes from an other thread!");
</ins><span class="cx"> return string;
</span><span class="cx"> }
</span><span class="cx"> return addSlowCase(string);
</span></span></pre></div>
<a id="trunkSourceWTFwtftextStringImplcpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/WTF/wtf/text/StringImpl.cpp (165945 => 165946)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WTF/wtf/text/StringImpl.cpp 2014-03-20 06:17:09 UTC (rev 165945)
+++ trunk/Source/WTF/wtf/text/StringImpl.cpp 2014-03-20 06:21:49 UTC (rev 165946)
</span><span class="lines">@@ -113,9 +113,9 @@
</span><span class="cx">
</span><span class="cx"> STRING_STATS_REMOVE_STRING(this);
</span><span class="cx">
</span><del>- if (isAtomic())
</del><ins>+ if (isAtomic() && m_length)
</ins><span class="cx"> AtomicString::remove(this);
</span><del>- if (isIdentifier()) {
</del><ins>+ if (isIdentifier() && m_length) {
</ins><span class="cx"> if (!wtfThreadData().currentIdentifierTable()->remove(this))
</span><span class="cx"> CRASH();
</span><span class="cx"> }
</span></span></pre></div>
<a id="trunkSourceWTFwtftextStringImplh"></a>
<div class="modfile"><h4>Modified: trunk/Source/WTF/wtf/text/StringImpl.h (165945 => 165946)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WTF/wtf/text/StringImpl.h 2014-03-20 06:17:09 UTC (rev 165945)
+++ trunk/Source/WTF/wtf/text/StringImpl.h 2014-03-20 06:21:49 UTC (rev 165946)
</span><span class="lines">@@ -293,18 +293,9 @@
</span><span class="cx"> // We expect m_length to be initialized to 0 as we use it
</span><span class="cx"> // to represent a null terminated buffer.
</span><span class="cx"> , m_data8(reinterpret_cast<const LChar*>(&m_length))
</span><ins>+ , m_hashAndFlags(hashAndFlagsForEmptyUnique())
</ins><span class="cx"> {
</span><span class="cx"> ASSERT(m_data8);
</span><del>- // Set the hash early, so that all empty unique StringImpls have a hash,
- // and don't use the normal hashing algorithm - the unique nature of these
- // keys means that we don't need them to match any other string (in fact,
- // that's exactly the oposite of what we want!), and teh normal hash would
- // lead to lots of conflicts.
- unsigned hash = cryptographicallyRandomNumber() | 1;
- hash <<= s_flagCount;
- if (!hash)
- hash = 1 << s_flagCount;
- m_hashAndFlags = hash | BufferInternal | s_hashFlag8BitBuffer;
</del><span class="cx">
</span><span class="cx"> STRING_STATS_ADD_8BIT_STRING(m_length);
</span><span class="cx"> }
</span><span class="lines">@@ -808,6 +799,7 @@
</span><span class="cx"> template <typename CharType> static PassRef<StringImpl> createInternal(const CharType*, unsigned);
</span><span class="cx"> WTF_EXPORT_STRING_API NEVER_INLINE const UChar* getData16SlowCase() const;
</span><span class="cx"> WTF_EXPORT_PRIVATE NEVER_INLINE unsigned hashSlowCase() const;
</span><ins>+ WTF_EXPORT_PRIVATE unsigned hashAndFlagsForEmptyUnique();
</ins><span class="cx">
</span><span class="cx"> // The bottom bit in the ref count indicates a static (immortal) string.
</span><span class="cx"> static const unsigned s_refCountFlagIsStaticString = 0x1;
</span></span></pre></div>
<a id="trunkSourceWTFwtftextStringStaticscpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/WTF/wtf/text/StringStatics.cpp (165945 => 165946)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WTF/wtf/text/StringStatics.cpp 2014-03-20 06:17:09 UTC (rev 165945)
+++ trunk/Source/WTF/wtf/text/StringStatics.cpp 2014-03-20 06:21:49 UTC (rev 165946)
</span><span class="lines">@@ -49,6 +49,19 @@
</span><span class="cx"> return &emptyString.get();
</span><span class="cx"> }
</span><span class="cx">
</span><ins>+// Set the hash early, so that all empty unique StringImpls have a hash,
+// and don't use the normal hashing algorithm - the unique nature of these
+// keys means that we don't need them to match any other string (in fact,
+// that's exactly the oposite of what we want!), and the normal hash would
+// lead to lots of conflicts.
+unsigned StringImpl::hashAndFlagsForEmptyUnique()
+{
+ static unsigned s_nextHashAndFlagsForEmptyUnique = BufferInternal | s_hashFlag8BitBuffer | s_hashFlagIsIdentifier | s_hashFlagIsAtomic;
+ s_nextHashAndFlagsForEmptyUnique += 1 << s_flagCount;
+ s_nextHashAndFlagsForEmptyUnique |= 1 << 31;
+ return s_nextHashAndFlagsForEmptyUnique;
+}
+
</ins><span class="cx"> WTF_EXPORTDATA DEFINE_GLOBAL(AtomicString, nullAtom)
</span><span class="cx"> WTF_EXPORTDATA DEFINE_GLOBAL(AtomicString, emptyAtom)
</span><span class="cx"> WTF_EXPORTDATA DEFINE_GLOBAL(AtomicString, textAtom)
</span></span></pre>
</div>
</div>
</body>
</html>