<!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>[199463] releases/WebKitGTK/webkit-2.12</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/199463">199463</a></dd>
<dt>Author</dt> <dd>carlosgc@webkit.org</dd>
<dt>Date</dt> <dd>2016-04-13 05:16:17 -0700 (Wed, 13 Apr 2016)</dd>
</dl>
<h3>Log Message</h3>
<pre>Merge <a href="http://trac.webkit.org/projects/webkit/changeset/198827">r198827</a> - [WTF] Removing a smart pointer from HashTable issues two stores to the same location
https://bugs.webkit.org/show_bug.cgi?id=155676
Patch by Benjamin Poulain <bpoulain@apple.com> on 2016-03-29
Reviewed by Darin Adler.
Source/WTF:
While working on the hot loop of <a href="http://trac.webkit.org/projects/webkit/changeset/198376">r198376</a>, I noticed something
weird...
Every time we removed a smart pointer from the hash table,
the code generated was something like:
Load([bucket]) -> Tmp
Store(0 -> [bucket])
JumpIfZero(Tmp, ->End)
Call fastFree()
Store(-1 -> [bucket])
-> End:
The useless store before the branch is annoying, especially on ARM.
Here is what happens:
1) The destructor of the smart pointer swaps its internal value with nullptr.
2) Since the smart pointer is not a local in-register value, that nullptr
is stored in memory because it could be observable from fastFree().
3) The destructor destroy the value if not zero (or deref for RefPtr).
The "if-not-zero" may or may not be eliminated depending on what
is between getting the iterator and the call to remove().
4) fastFree() is called.
5) The deleted value is set in the bucket.
This patch adds custom deletion for those cases to avoid the useless
store. The useless null check is still eliminated when we are lucky.
I went this path instead of changing the destructor of RefPtr for two reasons:
-I need this to work in unique_ptr for JSC.
-Nulling the memory may have security advantages in the cases where we do not immediately
write over that memory again.
This patch removes 13kb out of x86_64 WebCore.
* wtf/HashTable.h:
(WTF::HashTable::deleteBucket):
(WTF::KeyTraits>::removeIf):
* wtf/HashTraits.h:
(WTF::HashTraits<RefPtr<P>>::customDeleteBucket):
(WTF::hashTraitsDeleteBucket):
(WTF::KeyValuePairHashTraits::customDeleteBucket):
* wtf/text/AtomicStringHash.h:
(WTF::HashTraits<WTF::AtomicString>::isEmptyValue):
(WTF::HashTraits<WTF::AtomicString>::customDeleteBucket):
* wtf/text/StringHash.h:
(WTF::HashTraits<String>::customDeleteBucket):
Tools:
* TestWebKitAPI/Tests/WTF/HashMap.cpp:
* TestWebKitAPI/Tests/WTF/HashSet.cpp:</pre>
<h3>Modified Paths</h3>
<ul>
<li><a href="#releasesWebKitGTKwebkit212SourceWTFChangeLog">releases/WebKitGTK/webkit-2.12/Source/WTF/ChangeLog</a></li>
<li><a href="#releasesWebKitGTKwebkit212SourceWTFwtfHashTableh">releases/WebKitGTK/webkit-2.12/Source/WTF/wtf/HashTable.h</a></li>
<li><a href="#releasesWebKitGTKwebkit212SourceWTFwtfHashTraitsh">releases/WebKitGTK/webkit-2.12/Source/WTF/wtf/HashTraits.h</a></li>
<li><a href="#releasesWebKitGTKwebkit212SourceWTFwtftextAtomicStringHashh">releases/WebKitGTK/webkit-2.12/Source/WTF/wtf/text/AtomicStringHash.h</a></li>
<li><a href="#releasesWebKitGTKwebkit212SourceWTFwtftextStringHashh">releases/WebKitGTK/webkit-2.12/Source/WTF/wtf/text/StringHash.h</a></li>
<li><a href="#releasesWebKitGTKwebkit212ToolsChangeLog">releases/WebKitGTK/webkit-2.12/Tools/ChangeLog</a></li>
<li><a href="#releasesWebKitGTKwebkit212ToolsTestWebKitAPITestsWTFHashMapcpp">releases/WebKitGTK/webkit-2.12/Tools/TestWebKitAPI/Tests/WTF/HashMap.cpp</a></li>
<li><a href="#releasesWebKitGTKwebkit212ToolsTestWebKitAPITestsWTFHashSetcpp">releases/WebKitGTK/webkit-2.12/Tools/TestWebKitAPI/Tests/WTF/HashSet.cpp</a></li>
</ul>
</div>
<div id="patch">
<h3>Diff</h3>
<a id="releasesWebKitGTKwebkit212SourceWTFChangeLog"></a>
<div class="modfile"><h4>Modified: releases/WebKitGTK/webkit-2.12/Source/WTF/ChangeLog (199462 => 199463)</h4>
<pre class="diff"><span>
<span class="info">--- releases/WebKitGTK/webkit-2.12/Source/WTF/ChangeLog 2016-04-13 11:41:33 UTC (rev 199462)
+++ releases/WebKitGTK/webkit-2.12/Source/WTF/ChangeLog 2016-04-13 12:16:17 UTC (rev 199463)
</span><span class="lines">@@ -1,3 +1,56 @@
</span><ins>+2016-03-29 Benjamin Poulain <bpoulain@apple.com>
+
+ [WTF] Removing a smart pointer from HashTable issues two stores to the same location
+ https://bugs.webkit.org/show_bug.cgi?id=155676
+
+ Reviewed by Darin Adler.
+
+ While working on the hot loop of r198376, I noticed something
+ weird...
+ Every time we removed a smart pointer from the hash table,
+ the code generated was something like:
+ Load([bucket]) -> Tmp
+ Store(0 -> [bucket])
+ JumpIfZero(Tmp, ->End)
+ Call fastFree()
+ Store(-1 -> [bucket])
+ -> End:
+
+ The useless store before the branch is annoying, especially on ARM.
+
+ Here is what happens:
+ 1) The destructor of the smart pointer swaps its internal value with nullptr.
+ 2) Since the smart pointer is not a local in-register value, that nullptr
+ is stored in memory because it could be observable from fastFree().
+ 3) The destructor destroy the value if not zero (or deref for RefPtr).
+ The "if-not-zero" may or may not be eliminated depending on what
+ is between getting the iterator and the call to remove().
+ 4) fastFree() is called.
+ 5) The deleted value is set in the bucket.
+
+ This patch adds custom deletion for those cases to avoid the useless
+ store. The useless null check is still eliminated when we are lucky.
+
+ I went this path instead of changing the destructor of RefPtr for two reasons:
+ -I need this to work in unique_ptr for JSC.
+ -Nulling the memory may have security advantages in the cases where we do not immediately
+ write over that memory again.
+
+ This patch removes 13kb out of x86_64 WebCore.
+
+ * wtf/HashTable.h:
+ (WTF::HashTable::deleteBucket):
+ (WTF::KeyTraits>::removeIf):
+ * wtf/HashTraits.h:
+ (WTF::HashTraits<RefPtr<P>>::customDeleteBucket):
+ (WTF::hashTraitsDeleteBucket):
+ (WTF::KeyValuePairHashTraits::customDeleteBucket):
+ * wtf/text/AtomicStringHash.h:
+ (WTF::HashTraits<WTF::AtomicString>::isEmptyValue):
+ (WTF::HashTraits<WTF::AtomicString>::customDeleteBucket):
+ * wtf/text/StringHash.h:
+ (WTF::HashTraits<String>::customDeleteBucket):
+
</ins><span class="cx"> 2016-03-29 Yusuke Suzuki <utatane.tea@gmail.com>
</span><span class="cx">
</span><span class="cx"> REGRESSION(r192914): 10% regression on Sunspider's date-format-tofte
</span></span></pre></div>
<a id="releasesWebKitGTKwebkit212SourceWTFwtfHashTableh"></a>
<div class="modfile"><h4>Modified: releases/WebKitGTK/webkit-2.12/Source/WTF/wtf/HashTable.h (199462 => 199463)</h4>
<pre class="diff"><span>
<span class="info">--- releases/WebKitGTK/webkit-2.12/Source/WTF/wtf/HashTable.h 2016-04-13 11:41:33 UTC (rev 199462)
+++ releases/WebKitGTK/webkit-2.12/Source/WTF/wtf/HashTable.h 2016-04-13 12:16:17 UTC (rev 199463)
</span><span class="lines">@@ -456,7 +456,7 @@
</span><span class="cx"> ValueType* reinsert(ValueType&&);
</span><span class="cx">
</span><span class="cx"> static void initializeBucket(ValueType& bucket);
</span><del>- static void deleteBucket(ValueType& bucket) { bucket.~ValueType(); Traits::constructDeletedValue(bucket); }
</del><ins>+ static void deleteBucket(ValueType& bucket) { hashTraitsDeleteBucket<Traits>(bucket); }
</ins><span class="cx">
</span><span class="cx"> FullLookupType makeLookupResult(ValueType* position, bool found, unsigned hash)
</span><span class="cx"> { return FullLookupType(LookupType(position, found), hash); }
</span><span class="lines">@@ -1108,18 +1108,26 @@
</span><span class="cx"> template<typename Functor>
</span><span class="cx"> inline void HashTable<Key, Value, Extractor, HashFunctions, Traits, KeyTraits>::removeIf(const Functor& functor)
</span><span class="cx"> {
</span><ins>+ // We must use local copies in case "functor" or "deleteBucket"
+ // make a function call, which prevents the compiler from keeping
+ // the values in register.
+ unsigned removedBucketCount = 0;
+ ValueType* table = m_table;
+
</ins><span class="cx"> for (unsigned i = m_tableSize; i--;) {
</span><del>- if (isEmptyOrDeletedBucket(m_table[i]))
</del><ins>+ ValueType& bucket = table[i];
+ if (isEmptyOrDeletedBucket(bucket))
</ins><span class="cx"> continue;
</span><span class="cx">
</span><del>- if (!functor(m_table[i]))
</del><ins>+ if (!functor(bucket))
</ins><span class="cx"> continue;
</span><span class="cx">
</span><del>- deleteBucket(m_table[i]);
- ++m_deletedCount;
- --m_keyCount;
</del><ins>+ deleteBucket(bucket);
+ ++removedBucketCount;
</ins><span class="cx"> }
</span><del>-
</del><ins>+ m_deletedCount += removedBucketCount;
+ m_keyCount -= removedBucketCount;
+
</ins><span class="cx"> if (shouldShrink())
</span><span class="cx"> shrink();
</span><span class="cx">
</span></span></pre></div>
<a id="releasesWebKitGTKwebkit212SourceWTFwtfHashTraitsh"></a>
<div class="modfile"><h4>Modified: releases/WebKitGTK/webkit-2.12/Source/WTF/wtf/HashTraits.h (199462 => 199463)</h4>
<pre class="diff"><span>
<span class="info">--- releases/WebKitGTK/webkit-2.12/Source/WTF/wtf/HashTraits.h 2016-04-13 11:41:33 UTC (rev 199462)
+++ releases/WebKitGTK/webkit-2.12/Source/WTF/wtf/HashTraits.h 2016-04-13 12:16:17 UTC (rev 199463)
</span><span class="lines">@@ -116,6 +116,30 @@
</span><span class="cx"> typedef T* PeekType;
</span><span class="cx"> static T* peek(const std::unique_ptr<T, Deleter>& value) { return value.get(); }
</span><span class="cx"> static T* peek(std::nullptr_t) { return nullptr; }
</span><ins>+
+ static void customDeleteBucket(std::unique_ptr<T, Deleter>& value)
+ {
+ // The custom delete function exists to avoid a dead store before the value is destructed.
+ // The normal destruction sequence of a bucket would be:
+ // 1) Call the destructor of unique_ptr.
+ // 2) unique_ptr store a zero for its internal pointer.
+ // 3) unique_ptr destroys its value.
+ // 4) Call constructDeletedValue() to set the bucket as destructed.
+ //
+ // The problem is the call in (3) prevents the compile from eliminating the dead store in (2)
+ // becase a side effect of free() could be observing the value.
+ //
+ // This version of deleteBucket() ensures the dead 2 stores changing "value"
+ // are on the same side of the function call.
+ ASSERT(!isDeletedValue(value));
+ T* pointer = value.release();
+ constructDeletedValue(value);
+
+ // The null case happens if a caller uses std::move() to remove the pointer before calling remove()
+ // with an iterator. This is very uncommon.
+ if (LIKELY(pointer))
+ Deleter()(pointer);
+ }
</ins><span class="cx"> };
</span><span class="cx">
</span><span class="cx"> template<typename P> struct HashTraits<RefPtr<P>> : SimpleClassHashTraits<RefPtr<P>> {
</span><span class="lines">@@ -124,11 +148,21 @@
</span><span class="cx"> typedef P* PeekType;
</span><span class="cx"> static PeekType peek(const RefPtr<P>& value) { return value.get(); }
</span><span class="cx"> static PeekType peek(P* value) { return value; }
</span><ins>+
+ static void customDeleteBucket(RefPtr<P>& value)
+ {
+ // See unique_ptr's customDeleteBucket() for an explanation.
+ ASSERT(!SimpleClassHashTraits<RefPtr<P>>::isDeletedValue(value));
+ auto valueToBeDestroyed = WTFMove(value);
+ SimpleClassHashTraits<RefPtr<P>>::constructDeletedValue(value);
+ }
</ins><span class="cx"> };
</span><span class="cx">
</span><span class="cx"> template<> struct HashTraits<String> : SimpleClassHashTraits<String> {
</span><span class="cx"> static const bool hasIsEmptyValueFunction = true;
</span><span class="cx"> static bool isEmptyValue(const String&);
</span><ins>+
+ static void customDeleteBucket(String&);
</ins><span class="cx"> };
</span><span class="cx">
</span><span class="cx"> // This struct template is an implementation detail of the isHashTraitsEmptyValue function,
</span><span class="lines">@@ -145,6 +179,30 @@
</span><span class="cx"> return HashTraitsEmptyValueChecker<Traits, Traits::hasIsEmptyValueFunction>::isEmptyValue(value);
</span><span class="cx"> }
</span><span class="cx">
</span><ins>+template<typename Traits, typename T>
+struct HashTraitHasCustomDelete {
+ static T& bucketArg;
+ template<typename X> static std::true_type TestHasCustomDelete(X*, decltype(X::customDeleteBucket(bucketArg))* = nullptr);
+ static std::false_type TestHasCustomDelete(...);
+ typedef decltype(TestHasCustomDelete(static_cast<Traits*>(nullptr))) ResultType;
+ static const bool value = ResultType::value;
+};
+
+template<typename Traits, typename T>
+typename std::enable_if<HashTraitHasCustomDelete<Traits, T>::value>::type
+hashTraitsDeleteBucket(T& value)
+{
+ Traits::customDeleteBucket(value);
+}
+
+template<typename Traits, typename T>
+typename std::enable_if<!HashTraitHasCustomDelete<Traits, T>::value>::type
+hashTraitsDeleteBucket(T& value)
+{
+ value.~T();
+ Traits::constructDeletedValue(value);
+}
+
</ins><span class="cx"> template<typename FirstTraitsArg, typename SecondTraitsArg>
</span><span class="cx"> struct PairHashTraits : GenericHashTraits<std::pair<typename FirstTraitsArg::TraitType, typename SecondTraitsArg::TraitType>> {
</span><span class="cx"> typedef FirstTraitsArg FirstTraits;
</span><span class="lines">@@ -196,6 +254,7 @@
</span><span class="cx"> typedef ValueTraitsArg ValueTraits;
</span><span class="cx"> typedef KeyValuePair<typename KeyTraits::TraitType, typename ValueTraits::TraitType> TraitType;
</span><span class="cx"> typedef KeyValuePair<typename KeyTraits::EmptyValueType, typename ValueTraits::EmptyValueType> EmptyValueType;
</span><ins>+ typedef typename ValueTraitsArg::TraitType ValueType;
</ins><span class="cx">
</span><span class="cx"> static const bool emptyValueIsZero = KeyTraits::emptyValueIsZero && ValueTraits::emptyValueIsZero;
</span><span class="cx"> static EmptyValueType emptyValue() { return KeyValuePair<typename KeyTraits::EmptyValueType, typename ValueTraits::EmptyValueType>(KeyTraits::emptyValue(), ValueTraits::emptyValue()); }
</span><span class="lines">@@ -204,6 +263,15 @@
</span><span class="cx">
</span><span class="cx"> static void constructDeletedValue(TraitType& slot) { KeyTraits::constructDeletedValue(slot.key); }
</span><span class="cx"> static bool isDeletedValue(const TraitType& value) { return KeyTraits::isDeletedValue(value.key); }
</span><ins>+
+ static void customDeleteBucket(TraitType& value)
+ {
+ static_assert(std::is_trivially_destructible<KeyValuePair<int, int>>::value,
+ "The wrapper itself has to be trivially destructible for customDeleteBucket() to make sense, since we do not destruct the wrapper itself.");
+
+ hashTraitsDeleteBucket<KeyTraits>(value.key);
+ value.value.~ValueType();
+ }
</ins><span class="cx"> };
</span><span class="cx">
</span><span class="cx"> template<typename Key, typename Value>
</span></span></pre></div>
<a id="releasesWebKitGTKwebkit212SourceWTFwtftextAtomicStringHashh"></a>
<div class="modfile"><h4>Modified: releases/WebKitGTK/webkit-2.12/Source/WTF/wtf/text/AtomicStringHash.h (199462 => 199463)</h4>
<pre class="diff"><span>
<span class="info">--- releases/WebKitGTK/webkit-2.12/Source/WTF/wtf/text/AtomicStringHash.h 2016-04-13 11:41:33 UTC (rev 199462)
+++ releases/WebKitGTK/webkit-2.12/Source/WTF/wtf/text/AtomicStringHash.h 2016-04-13 12:16:17 UTC (rev 199463)
</span><span class="lines">@@ -48,9 +48,22 @@
</span><span class="cx"> static const bool safeToCompareToEmptyOrDeleted = false;
</span><span class="cx"> };
</span><span class="cx">
</span><del>- // AtomicStringHash is the default hash for AtomicString
- template<> struct HashTraits<WTF::AtomicString> : SimpleClassHashTraits<WTF::AtomicString> { };
</del><ins>+ template<> struct HashTraits<WTF::AtomicString> : SimpleClassHashTraits<WTF::AtomicString> {
+ static const bool hasIsEmptyValueFunction = true;
+ static bool isEmptyValue(const AtomicString& value)
+ {
+ return value.isNull();
+ }
</ins><span class="cx">
</span><ins>+ static void customDeleteBucket(AtomicString& value)
+ {
+ // See unique_ptr's customDeleteBucket() for an explanation.
+ ASSERT(!isDeletedValue(value));
+ AtomicString valueToBeDestroyed = WTFMove(value);
+ constructDeletedValue(value);
+ }
+ };
+
</ins><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> using WTF::AtomicStringHash;
</span></span></pre></div>
<a id="releasesWebKitGTKwebkit212SourceWTFwtftextStringHashh"></a>
<div class="modfile"><h4>Modified: releases/WebKitGTK/webkit-2.12/Source/WTF/wtf/text/StringHash.h (199462 => 199463)</h4>
<pre class="diff"><span>
<span class="info">--- releases/WebKitGTK/webkit-2.12/Source/WTF/wtf/text/StringHash.h 2016-04-13 11:41:33 UTC (rev 199462)
+++ releases/WebKitGTK/webkit-2.12/Source/WTF/wtf/text/StringHash.h 2016-04-13 12:16:17 UTC (rev 199463)
</span><span class="lines">@@ -33,6 +33,14 @@
</span><span class="cx"> return value.isNull();
</span><span class="cx"> }
</span><span class="cx">
</span><ins>+ inline void HashTraits<String>::customDeleteBucket(String& value)
+ {
+ // See unique_ptr's customDeleteBucket() for an explanation.
+ ASSERT(!isDeletedValue(value));
+ String valueToBeDestroyed = WTFMove(value);
+ constructDeletedValue(value);
+ }
+
</ins><span class="cx"> // The hash() functions on StringHash and ASCIICaseInsensitiveHash do not support
</span><span class="cx"> // null strings. get(), contains(), and add() on HashMap<String,..., StringHash>
</span><span class="cx"> // cause a null-pointer dereference when passed null strings.
</span></span></pre></div>
<a id="releasesWebKitGTKwebkit212ToolsChangeLog"></a>
<div class="modfile"><h4>Modified: releases/WebKitGTK/webkit-2.12/Tools/ChangeLog (199462 => 199463)</h4>
<pre class="diff"><span>
<span class="info">--- releases/WebKitGTK/webkit-2.12/Tools/ChangeLog 2016-04-13 11:41:33 UTC (rev 199462)
+++ releases/WebKitGTK/webkit-2.12/Tools/ChangeLog 2016-04-13 12:16:17 UTC (rev 199463)
</span><span class="lines">@@ -1,3 +1,13 @@
</span><ins>+2016-03-29 Benjamin Poulain <bpoulain@apple.com>
+
+ [WTF] Removing a smart pointer from HashTable issues two stores to the same location
+ https://bugs.webkit.org/show_bug.cgi?id=155676
+
+ Reviewed by Darin Adler.
+
+ * TestWebKitAPI/Tests/WTF/HashMap.cpp:
+ * TestWebKitAPI/Tests/WTF/HashSet.cpp:
+
</ins><span class="cx"> 2016-03-21 Brent Fulgham <bfulgham@apple.com>
</span><span class="cx">
</span><span class="cx"> [Win] SharedBuffer::copy() can cause a segmentation fault.
</span></span></pre></div>
<a id="releasesWebKitGTKwebkit212ToolsTestWebKitAPITestsWTFHashMapcpp"></a>
<div class="modfile"><h4>Modified: releases/WebKitGTK/webkit-2.12/Tools/TestWebKitAPI/Tests/WTF/HashMap.cpp (199462 => 199463)</h4>
<pre class="diff"><span>
<span class="info">--- releases/WebKitGTK/webkit-2.12/Tools/TestWebKitAPI/Tests/WTF/HashMap.cpp 2016-04-13 11:41:33 UTC (rev 199462)
+++ releases/WebKitGTK/webkit-2.12/Tools/TestWebKitAPI/Tests/WTF/HashMap.cpp 2016-04-13 12:16:17 UTC (rev 199463)
</span><span class="lines">@@ -570,4 +570,133 @@
</span><span class="cx"> }
</span><span class="cx"> }
</span><span class="cx">
</span><ins>+class ObjectWithRefLogger {
+public:
+ ObjectWithRefLogger(Ref<RefLogger>&& logger)
+ : m_logger(WTFMove(logger))
+ {
+ }
+
+ Ref<RefLogger> m_logger;
+};
+
+
+void testMovingUsingEnsure(Ref<RefLogger>&& logger)
+{
+ HashMap<unsigned, std::unique_ptr<ObjectWithRefLogger>> map;
+
+ map.ensure(1, [&] { return std::make_unique<ObjectWithRefLogger>(WTFMove(logger)); });
+}
+
+void testMovingUsingAdd(Ref<RefLogger>&& logger)
+{
+ HashMap<unsigned, std::unique_ptr<ObjectWithRefLogger>> map;
+
+ auto& slot = map.add(1, nullptr).iterator->value;
+ slot = std::make_unique<ObjectWithRefLogger>(WTFMove(logger));
+}
+
+TEST(WTF_HashMap, Ensure_LambdasCapturingByReference)
+{
+ {
+ DerivedRefLogger a("a");
+ Ref<RefLogger> ref(a);
+ testMovingUsingEnsure(WTFMove(ref));
+
+ EXPECT_STREQ("ref(a) deref(a) ", takeLogStr().c_str());
+ }
+
+ {
+ DerivedRefLogger a("a");
+ Ref<RefLogger> ref(a);
+ testMovingUsingAdd(WTFMove(ref));
+
+ EXPECT_STREQ("ref(a) deref(a) ", takeLogStr().c_str());
+ }
+}
+
+
+TEST(WTF_HashMap, ValueIsDestructedOnRemove)
+{
+ struct DestructorObserver {
+ DestructorObserver() = default;
+
+ DestructorObserver(bool* destructed)
+ : destructed(destructed)
+ {
+ }
+
+ ~DestructorObserver()
+ {
+ if (destructed)
+ *destructed = true;
+ }
+
+ DestructorObserver(DestructorObserver&& other)
+ : destructed(other.destructed)
+ {
+ other.destructed = nullptr;
+ }
+
+ DestructorObserver& operator=(DestructorObserver&& other)
+ {
+ destructed = other.destructed;
+ other.destructed = nullptr;
+ return *this;
+ }
+
+ bool* destructed { nullptr };
+ };
+
+ HashMap<int, DestructorObserver> map;
+
+ bool destructed = false;
+ map.add(5, DestructorObserver { &destructed });
+
+ EXPECT_FALSE(destructed);
+
+ bool removeResult = map.remove(5);
+
+ EXPECT_TRUE(removeResult);
+ EXPECT_TRUE(destructed);
+}
+
+TEST(WTF_HashMap, RefPtrNotZeroedBeforeDeref)
+{
+ struct DerefObserver {
+ NEVER_INLINE void ref()
+ {
+ ++count;
+ }
+ NEVER_INLINE void deref()
+ {
+ --count;
+ observedBucket = bucketAddress->get();
+ }
+ unsigned count { 1 };
+ const RefPtr<DerefObserver>* bucketAddress { nullptr };
+ const DerefObserver* observedBucket { nullptr };
+ };
+
+ auto observer = std::make_unique<DerefObserver>();
+
+ HashMap<RefPtr<DerefObserver>, int> map;
+ map.add(adoptRef(observer.get()), 5);
+
+ auto iterator = map.find(observer.get());
+ EXPECT_TRUE(iterator != map.end());
+
+ observer->bucketAddress = &iterator->key;
+
+ EXPECT_TRUE(observer->observedBucket == nullptr);
+ EXPECT_TRUE(map.remove(observer.get()));
+
+ // It if fine to either leave the old value intact at deletion or already set it to the deleted
+ // value.
+ // A zero would be a incorrect outcome as it would mean we nulled the bucket before an opaque
+ // call.
+ EXPECT_TRUE(observer->observedBucket == observer.get() || observer->observedBucket == RefPtr<DerefObserver>::hashTableDeletedValue());
+ EXPECT_EQ(observer->count, 0u);
+}
+
</ins><span class="cx"> } // namespace TestWebKitAPI
</span></span></pre></div>
<a id="releasesWebKitGTKwebkit212ToolsTestWebKitAPITestsWTFHashSetcpp"></a>
<div class="modfile"><h4>Modified: releases/WebKitGTK/webkit-2.12/Tools/TestWebKitAPI/Tests/WTF/HashSet.cpp (199462 => 199463)</h4>
<pre class="diff"><span>
<span class="info">--- releases/WebKitGTK/webkit-2.12/Tools/TestWebKitAPI/Tests/WTF/HashSet.cpp 2016-04-13 11:41:33 UTC (rev 199462)
+++ releases/WebKitGTK/webkit-2.12/Tools/TestWebKitAPI/Tests/WTF/HashSet.cpp 2016-04-13 12:16:17 UTC (rev 199463)
</span><span class="lines">@@ -28,6 +28,7 @@
</span><span class="cx"> #include "Counters.h"
</span><span class="cx"> #include "MoveOnly.h"
</span><span class="cx"> #include <wtf/HashSet.h>
</span><ins>+#include <wtf/RefPtr.h>
</ins><span class="cx">
</span><span class="cx"> namespace TestWebKitAPI {
</span><span class="cx">
</span><span class="lines">@@ -276,5 +277,76 @@
</span><span class="cx"> }
</span><span class="cx"> }
</span><span class="cx">
</span><ins>+TEST(WTF_HashSet, RefPtrNotZeroedBeforeDeref)
+{
+ struct DerefObserver {
+ NEVER_INLINE void ref()
+ {
+ ++count;
+ }
+ NEVER_INLINE void deref()
+ {
+ --count;
+ observedBucket = bucketAddress->get();
+ }
+ unsigned count { 1 };
+ const RefPtr<DerefObserver>* bucketAddress { nullptr };
+ const DerefObserver* observedBucket { nullptr };
+ };
</ins><span class="cx">
</span><ins>+ auto observer = std::make_unique<DerefObserver>();
+
+ HashSet<RefPtr<DerefObserver>> set;
+ set.add(adoptRef(observer.get()));
+
+ auto iterator = set.find(observer.get());
+ EXPECT_TRUE(iterator != set.end());
+
+ observer->bucketAddress = iterator.get();
+
+ EXPECT_TRUE(observer->observedBucket == nullptr);
+ EXPECT_TRUE(set.remove(observer.get()));
+
+ // It if fine to either leave the old value intact at deletion or already set it to the deleted
+ // value.
+ // A zero would be a incorrect outcome as it would mean we nulled the bucket before an opaque
+ // call.
+ EXPECT_TRUE(observer->observedBucket == observer.get() || observer->observedBucket == RefPtr<DerefObserver>::hashTableDeletedValue());
+ EXPECT_EQ(observer->count, 0u);
+}
+
+
+TEST(WTF_HashSet, UniquePtrNotZeroedBeforeDestructor)
+{
+ struct DestructorObserver {
+ ~DestructorObserver()
+ {
+ observe();
+ }
+ std::function<void()> observe;
+ };
+
+ const std::unique_ptr<DestructorObserver>* bucketAddress = nullptr;
+ const DestructorObserver* observedBucket = nullptr;
+ std::unique_ptr<DestructorObserver> observer(new DestructorObserver { [&]() {
+ observedBucket = bucketAddress->get();
+ }});
+
+ const DestructorObserver* observerAddress = observer.get();
+
+ HashSet<std::unique_ptr<DestructorObserver>> set;
+ auto addResult = set.add(WTFMove(observer));
+
+ EXPECT_TRUE(addResult.isNewEntry);
+ EXPECT_TRUE(observedBucket == nullptr);
+
+ bucketAddress = addResult.iterator.get();
+
+ EXPECT_TRUE(observedBucket == nullptr);
+ EXPECT_TRUE(set.remove(*addResult.iterator));
+
+ EXPECT_TRUE(observedBucket == observerAddress || observedBucket == reinterpret_cast<const DestructorObserver*>(-1));
+}
+
+
</ins><span class="cx"> } // namespace TestWebKitAPI
</span></span></pre>
</div>
</div>
</body>
</html>