- Revision
- 234879
- Author
- [email protected]
- Date
- 2018-08-14 21:08:08 -0700 (Tue, 14 Aug 2018)
Log Message
HashMap<Ref<P>, V> asserts when V is not zero for its empty value
https://bugs.webkit.org/show_bug.cgi?id=188582
Reviewed by Sam Weinig.
Source/_javascript_Core:
* runtime/SparseArrayValueMap.h:
Source/WTF:
The issue happened when we'd fill the hash table buffer with empty values. We
would iterate the buffer and invoke placement new with the incoming value being the
empty value. For Ref, this means that, we'd call its move constructor, which calls
leakRef(), which asserts that the Ref's pointer is not null. We'd like to keep
this assert since it catches bugs where you leakRef() more than once or WTFMove
an already moved Ref.
This patch fixes this issue by adding a new trait for constructing an empty
value. We use that in HashTable instead of directly calling placement new.
* wtf/HashTable.h:
(WTF::HashTableBucketInitializer<false>::initialize):
* wtf/HashTraits.h:
(WTF::GenericHashTraits::constructEmptyValue):
(WTF::HashTraits<Ref<P>>::constructEmptyValue):
(WTF::KeyValuePairHashTraits::constructEmptyValue):
Tools:
* TestWebKitAPI/Tests/WTF/HashMap.cpp:
(TestWebKitAPI::TEST):
Modified Paths
Diff
Modified: trunk/Source/_javascript_Core/ChangeLog (234878 => 234879)
--- trunk/Source/_javascript_Core/ChangeLog 2018-08-15 02:59:51 UTC (rev 234878)
+++ trunk/Source/_javascript_Core/ChangeLog 2018-08-15 04:08:08 UTC (rev 234879)
@@ -1,3 +1,12 @@
+2018-08-14 Saam barati <[email protected]>
+
+ HashMap<Ref<P>, V> asserts when V is not zero for its empty value
+ https://bugs.webkit.org/show_bug.cgi?id=188582
+
+ Reviewed by Sam Weinig.
+
+ * runtime/SparseArrayValueMap.h:
+
2018-08-14 Yusuke Suzuki <[email protected]>
Unreviewed, attempt to fix CLoop build
Modified: trunk/Source/_javascript_Core/runtime/SparseArrayValueMap.h (234878 => 234879)
--- trunk/Source/_javascript_Core/runtime/SparseArrayValueMap.h 2018-08-15 02:59:51 UTC (rev 234878)
+++ trunk/Source/_javascript_Core/runtime/SparseArrayValueMap.h 2018-08-15 04:08:08 UTC (rev 234879)
@@ -37,6 +37,7 @@
class SparseArrayValueMap;
class SparseArrayEntry : private WriteBarrier<Unknown> {
+ WTF_MAKE_FAST_ALLOCATED;
public:
using Base = WriteBarrier<Unknown>;
Modified: trunk/Source/WTF/ChangeLog (234878 => 234879)
--- trunk/Source/WTF/ChangeLog 2018-08-15 02:59:51 UTC (rev 234878)
+++ trunk/Source/WTF/ChangeLog 2018-08-15 04:08:08 UTC (rev 234879)
@@ -1,3 +1,27 @@
+2018-08-14 Saam barati <[email protected]>
+
+ HashMap<Ref<P>, V> asserts when V is not zero for its empty value
+ https://bugs.webkit.org/show_bug.cgi?id=188582
+
+ Reviewed by Sam Weinig.
+
+ The issue happened when we'd fill the hash table buffer with empty values. We
+ would iterate the buffer and invoke placement new with the incoming value being the
+ empty value. For Ref, this means that, we'd call its move constructor, which calls
+ leakRef(), which asserts that the Ref's pointer is not null. We'd like to keep
+ this assert since it catches bugs where you leakRef() more than once or WTFMove
+ an already moved Ref.
+
+ This patch fixes this issue by adding a new trait for constructing an empty
+ value. We use that in HashTable instead of directly calling placement new.
+
+ * wtf/HashTable.h:
+ (WTF::HashTableBucketInitializer<false>::initialize):
+ * wtf/HashTraits.h:
+ (WTF::GenericHashTraits::constructEmptyValue):
+ (WTF::HashTraits<Ref<P>>::constructEmptyValue):
+ (WTF::KeyValuePairHashTraits::constructEmptyValue):
+
2018-08-14 Fujii Hironori <[email protected]>
Unreviewed, rolling out r234859.
Modified: trunk/Source/WTF/wtf/HashTable.h (234878 => 234879)
--- trunk/Source/WTF/wtf/HashTable.h 2018-08-15 02:59:51 UTC (rev 234878)
+++ trunk/Source/WTF/wtf/HashTable.h 2018-08-15 04:08:08 UTC (rev 234879)
@@ -837,7 +837,7 @@
template<> struct HashTableBucketInitializer<false> {
template<typename Traits, typename Value> static void initialize(Value& bucket)
{
- new (NotNull, std::addressof(bucket)) Value(Traits::emptyValue());
+ Traits::template constructEmptyValue<Traits>(bucket);
}
};
Modified: trunk/Source/WTF/wtf/HashTraits.h (234878 => 234879)
--- trunk/Source/WTF/wtf/HashTraits.h 2018-08-15 02:59:51 UTC (rev 234878)
+++ trunk/Source/WTF/wtf/HashTraits.h 2018-08-15 04:08:08 UTC (rev 234879)
@@ -70,6 +70,12 @@
emptyValue = std::forward<V>(value);
}
+ template <typename Traits>
+ static void constructEmptyValue(T& slot)
+ {
+ new (NotNull, std::addressof(slot)) T(Traits::emptyValue());
+ }
+
// Type for return value of functions that do not transfer ownership, such as get.
typedef T PeekType;
template<typename U> static U&& peek(U&& value) { return std::forward<U>(value); }
@@ -191,6 +197,12 @@
static const bool emptyValueIsZero = true;
static Ref<P> emptyValue() { return HashTableEmptyValue; }
+ template <typename>
+ static void constructEmptyValue(Ref<P>& slot)
+ {
+ new (NotNull, std::addressof(slot)) Ref<P>(HashTableEmptyValue);
+ }
+
static const bool hasIsEmptyValueFunction = true;
static bool isEmptyValue(const Ref<P>& value) { return value.isHashTableEmptyValue(); }
@@ -302,6 +314,13 @@
static const bool emptyValueIsZero = KeyTraits::emptyValueIsZero && ValueTraits::emptyValueIsZero;
static EmptyValueType emptyValue() { return KeyValuePair<typename KeyTraits::EmptyValueType, typename ValueTraits::EmptyValueType>(KeyTraits::emptyValue(), ValueTraits::emptyValue()); }
+ template <typename>
+ static void constructEmptyValue(TraitType& slot)
+ {
+ KeyTraits::template constructEmptyValue<KeyTraits>(slot.key);
+ ValueTraits::template constructEmptyValue<ValueTraits>(slot.value);
+ }
+
static const unsigned minimumTableSize = KeyTraits::minimumTableSize;
static void constructDeletedValue(TraitType& slot) { KeyTraits::constructDeletedValue(slot.key); }
Modified: trunk/Tools/ChangeLog (234878 => 234879)
--- trunk/Tools/ChangeLog 2018-08-15 02:59:51 UTC (rev 234878)
+++ trunk/Tools/ChangeLog 2018-08-15 04:08:08 UTC (rev 234879)
@@ -1,3 +1,13 @@
+2018-08-14 Saam barati <[email protected]>
+
+ HashMap<Ref<P>, V> asserts when V is not zero for its empty value
+ https://bugs.webkit.org/show_bug.cgi?id=188582
+
+ Reviewed by Sam Weinig.
+
+ * TestWebKitAPI/Tests/WTF/HashMap.cpp:
+ (TestWebKitAPI::TEST):
+
2018-08-14 Zalan Bujtas <[email protected]>
[LFC][Floating] Add support for negative clearance.
Modified: trunk/Tools/TestWebKitAPI/Tests/WTF/HashMap.cpp (234878 => 234879)
--- trunk/Tools/TestWebKitAPI/Tests/WTF/HashMap.cpp 2018-08-15 02:59:51 UTC (rev 234878)
+++ trunk/Tools/TestWebKitAPI/Tests/WTF/HashMap.cpp 2018-08-15 04:08:08 UTC (rev 234879)
@@ -945,4 +945,47 @@
(void)value;
}
+TEST(WTF_HashMap, RefMappedToNonZeroEmptyValue)
+{
+ class Value {
+ public:
+ Value() = default;
+ Value(Value&&) = default;
+ Value(const Value&) = default;
+ Value& operator=(Value&&) = default;
+
+ Value(int32_t f)
+ : m_field(f)
+ { }
+
+ int32_t field() { return m_field; }
+
+ private:
+ int32_t m_field { 0xbadbeef };
+ };
+
+ class Key : public RefCounted<Key> {
+ Key() = default;
+ public:
+ static Ref<Key> create() { return adoptRef(*new Key); }
+ };
+
+ static_assert(!WTF::HashTraits<Value>::emptyValueIsZero, "");
+
+ HashMap<Ref<Key>, Value> map;
+ Vector<std::pair<Ref<Key>, int32_t>> vectorMap;
+
+ for (int32_t i = 0; i < 160; ++i) {
+ Ref<Key> key = Key::create();
+ map.add(Ref<Key>(key.get()), Value { i });
+ vectorMap.append({ WTFMove(key), i });
+ }
+
+ for (auto& pair : vectorMap)
+ ASSERT_EQ(pair.second, map.get(pair.first).field());
+
+ for (auto& pair : vectorMap)
+ ASSERT_TRUE(map.remove(pair.first));
+}
+
} // namespace TestWebKitAPI
