Attachment 347141 Details for Bug 188582 – patch

[patch] patch

b-backup.diff (text/plain), 6.99 KB, created by Saam Barati on 2018-08-14 18:25:16 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Saam Barati

Created: 2018-08-14 18:25:16 PDT

Size: 6.99 KB

patch

obsolete

>Index: Source/JavaScriptCore/ChangeLog
>===================================================================
>--- Source/JavaScriptCore/ChangeLog	(revision 234863)
>+++ Source/JavaScriptCore/ChangeLog	(working copy)
>@@ -1,3 +1,12 @@
>+2018-08-14 Saam barati <sbarati@apple.com>
>+
>+ HashMap<Ref, V> asserts when V is not zero for its empty value
>+ https://bugs.webkit.org/show_bug.cgi?id=188582
>+
>+ Reviewed by NOBODY (OOPS!).
>+
>+ * runtime/SparseArrayValueMap.h:
>+
> 2018-08-14 Keith Miller <keith_miller@apple.com>
> 
> Add missing availability macro.
>Index: Source/JavaScriptCore/runtime/SparseArrayValueMap.h
>===================================================================
>--- Source/JavaScriptCore/runtime/SparseArrayValueMap.h	(revision 234863)
>+++ Source/JavaScriptCore/runtime/SparseArrayValueMap.h	(working copy)
>@@ -37,6 +37,7 @@ namespace JSC {
> class SparseArrayValueMap;
> 
> class SparseArrayEntry : private WriteBarrier<Unknown> {
>+ WTF_MAKE_FAST_ALLOCATED;
> public:
> using Base = WriteBarrier<Unknown>;
> 
>Index: Source/WTF/ChangeLog
>===================================================================
>--- Source/WTF/ChangeLog	(revision 234863)
>+++ Source/WTF/ChangeLog	(working copy)
>@@ -1,3 +1,27 @@
>+2018-08-14 Saam barati <sbarati@apple.com>
>+
>+ HashMap<Ref, V> asserts when V is not zero for its empty value
>+ https://bugs.webkit.org/show_bug.cgi?id=188582
>+
>+ Reviewed by NOBODY (OOPS!).
>+
>+ 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>::constructEmptyValue):
>+ (WTF::KeyValuePairHashTraits::constructEmptyValue):
>+
> 2018-08-14 Alex Christensen <achristensen@webkit.org>
> 
> Use a Variant instead of a union in CSSSelector
>Index: Source/WTF/wtf/HashTable.h
>===================================================================
>--- Source/WTF/wtf/HashTable.h	(revision 234863)
>+++ Source/WTF/wtf/HashTable.h	(working copy)
>@@ -837,7 +837,7 @@ namespace WTF {
> 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);
> }
> };
> 
>Index: Source/WTF/wtf/HashTraits.h
>===================================================================
>--- Source/WTF/wtf/HashTraits.h	(revision 234863)
>+++ Source/WTF/wtf/HashTraits.h	(working copy)
>@@ -70,6 +70,12 @@ template<typename T> struct GenericHashT
> 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(value); }
>@@ -191,6 +197,12 @@ template<typename P> struct HashTraits<R
> static const bool emptyValueIsZero = true;
> static Ref emptyValue() { return HashTableEmptyValue; }
> 
>+ template <typename>
>+ static void constructEmptyValue(Ref& slot)
>+ {
>+ new (NotNull, std::addressof(slot)) Ref(HashTableEmptyValue);
>+ }
>+
> static const bool hasIsEmptyValueFunction = true;
> static bool isEmptyValue(const Ref& value) { return value.isHashTableEmptyValue(); }
> 
>@@ -302,6 +314,13 @@ struct KeyValuePairHashTraits : GenericH
> 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); }
>Index: Tools/ChangeLog
>===================================================================
>--- Tools/ChangeLog	(revision 234875)
>+++ Tools/ChangeLog	(working copy)
>@@ -1,3 +1,13 @@
>+2018-08-14 Saam barati <sbarati@apple.com>
>+
>+ HashMap<Ref, V> asserts when V is not zero for its empty value
>+ https://bugs.webkit.org/show_bug.cgi?id=188582
>+
>+ Reviewed by NOBODY (OOPS!).
>+
>+ * TestWebKitAPI/Tests/WTF/HashMap.cpp:
>+ (TestWebKitAPI::TEST):
>+
> 2018-08-14 Sihui Liu <sihui_liu@apple.com>
> 
> Crash in WebKit::filterPreloadHSTSEntry via NetworkProcess::getHostNamesWithHSTSCache
>Index: Tools/TestWebKitAPI/Tests/WTF/HashMap.cpp
>===================================================================
>--- Tools/TestWebKitAPI/Tests/WTF/HashMap.cpp	(revision 234863)
>+++ Tools/TestWebKitAPI/Tests/WTF/HashMap.cpp	(working copy)
>@@ -945,4 +945,47 @@ TEST(WTF_HashMap, DeletedAddressOfOperat
> (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

Actions: View | Formatted Diff | Diff

Attachments on bug 188582: 347141