- Revision
- 295092
- Author
- [email protected]
- Date
- 2022-06-01 09:58:41 -0700 (Wed, 01 Jun 2022)
Log Message
WeakHashMap::ensure() may crash if the map contains null references https://bugs.webkit.org/show_bug.cgi?id=241162
Reviewed by Geoffrey Garen.
WeakHashMap::ensure() may crash if the map contains null references, because
the WeakHashMap iterator destructor can clear null references and the AddResult
constructor copies and destroys the input iterator.
I find it very error-prone that destroying an iterator would modify the hash
map and thus invalidate other iterators (or even itself if the iterator was
merely copied). As a result, I removed this logic from the
WeakHashMapIteratorBase destructor. Instead, I now increase
WeakHashMap::m_operationCountSinceLastCleanup whenever the iterator gets
incremented so that null references will be removed the next time the hash map
is modified.
I also updated other read-only operations (such as get() / find() / contains())
to just increment m_operationCountSinceLastCleanup without actually clearing
null references for the same reason as above. Having such read-only operations
invalidate existing iterators is just too error-prone.
Finally, I updated the AddResult constructor to avoid copying the
WeakHashMapIterator it is passed, given that the WeakHashMapIterator
constructor and destructor do some work.
* Source/WTF/wtf/WeakHashMap.h:
* Source/WebCore/dom/Element.cpp:
(WebCore::Element::identifier const):
* Tools/TestWebKitAPI/Tests/WTF/WeakPtr.cpp:
(TestWebKitAPI::TEST):
Canonical link: https://commits.webkit.org/251187@main
Modified Paths
Diff
Modified: trunk/Source/WTF/wtf/WeakHashMap.h (295091 => 295092)
--- trunk/Source/WTF/wtf/WeakHashMap.h 2022-06-01 16:55:02 UTC (rev 295091)
+++ trunk/Source/WTF/wtf/WeakHashMap.h 2022-06-01 16:58:41 UTC (rev 295092)
@@ -89,13 +89,7 @@
skipEmptyBuckets();
}
- ~WeakHashMapIteratorBase()
- {
- if (m_emptyBucketCount > m_weakHashMap.m_map.size() / 8)
- const_cast<WeakHashMap&>(m_weakHashMap).removeNullReferences();
- else
- m_weakHashMap.amortizedCleanupIfNeeded(m_advanceCount + m_emptyBucketCount);
- }
+ ~WeakHashMapIteratorBase() = default;
ALWAYS_INLINE IteratorPeekType makePeek()
{
@@ -117,14 +111,13 @@
++m_position;
++m_advanceCount;
skipEmptyBuckets();
+ m_weakHashMap.increaseOperationCountSinceLastCleanup();
}
void skipEmptyBuckets()
{
- while (m_position != m_endPosition && !m_position->key.get()) {
+ while (m_position != m_endPosition && !m_position->key.get())
++m_position;
- ++m_emptyBucketCount;
- }
}
const MapType& m_weakHashMap;
@@ -131,7 +124,6 @@
IteratorType m_position;
IteratorType m_endPosition;
unsigned m_advanceCount { 0 };
- unsigned m_emptyBucketCount { 0 };
};
class WeakHashMapIterator : public WeakHashMapIteratorBase<WeakHashMap, typename WeakHashImplMap::iterator, PeekPtrType, PeekType> {
@@ -186,7 +178,10 @@
struct AddResult {
AddResult() : isNewEntry(false) { }
- AddResult(WeakHashMapIterator it, bool isNewEntry) : iterator(it), isNewEntry(isNewEntry) { }
+ AddResult(WeakHashMapIterator&& it, bool isNewEntry)
+ : iterator(WTFMove(it))
+ , isNewEntry(isNewEntry)
+ { }
WeakHashMapIterator iterator;
bool isNewEntry;
@@ -226,7 +221,7 @@
iterator find(const KeyType& key)
{
- amortizedCleanupIfNeeded();
+ increaseOperationCountSinceLastCleanup();
auto* keyImpl = keyImplIfExists(key);
if (!keyImpl)
return end();
@@ -235,7 +230,7 @@
const_iterator find(const KeyType& key) const
{
- amortizedCleanupIfNeeded();
+ increaseOperationCountSinceLastCleanup();
auto* keyImpl = keyImplIfExists(key);
if (!keyImpl)
return end();
@@ -244,7 +239,7 @@
bool contains(const KeyType& key) const
{
- amortizedCleanupIfNeeded();
+ increaseOperationCountSinceLastCleanup();
auto* keyImpl = keyImplIfExists(key);
if (!keyImpl)
return false;
@@ -262,7 +257,7 @@
typename ValueTraits::PeekType get(const KeyType& key)
{
- amortizedCleanupIfNeeded();
+ increaseOperationCountSinceLastCleanup();
auto* keyImpl = keyImplIfExists(key);
if (!keyImpl)
return ValueTraits::peek(ValueTraits::emptyValue());
@@ -347,10 +342,16 @@
#endif
private:
+ ALWAYS_INLINE unsigned increaseOperationCountSinceLastCleanup(unsigned operationsPerformed = 1) const
+ {
+ unsigned currentCount = m_operationCountSinceLastCleanup;
+ m_operationCountSinceLastCleanup += operationsPerformed;
+ return currentCount;
+ }
+
ALWAYS_INLINE void amortizedCleanupIfNeeded(unsigned operationsPerformed = 1) const
{
- unsigned currentCount = m_operationCountSinceLastCleanup;
- m_operationCountSinceLastCleanup = currentCount + operationsPerformed;
+ unsigned currentCount = increaseOperationCountSinceLastCleanup(operationsPerformed);
if (currentCount / 2 > m_map.size())
const_cast<WeakHashMap&>(*this).removeNullReferences();
}
Modified: trunk/Source/WebCore/dom/Element.cpp (295091 => 295092)
--- trunk/Source/WebCore/dom/Element.cpp 2022-06-01 16:55:02 UTC (rev 295091)
+++ trunk/Source/WebCore/dom/Element.cpp 2022-06-01 16:58:41 UTC (rev 295092)
@@ -4869,7 +4869,6 @@
ElementIdentifier Element::identifier() const
{
- elementIdentifiersMap().removeNullReferences();
return elementIdentifiersMap().ensure(*this, [] { return ElementIdentifier::generate(); }).iterator->value;
}
Modified: trunk/Tools/TestWebKitAPI/Tests/WTF/WeakPtr.cpp (295091 => 295092)
--- trunk/Tools/TestWebKitAPI/Tests/WTF/WeakPtr.cpp 2022-06-01 16:55:02 UTC (rev 295091)
+++ trunk/Tools/TestWebKitAPI/Tests/WTF/WeakPtr.cpp 2022-06-01 16:58:41 UTC (rev 295092)
@@ -1602,9 +1602,9 @@
auto pairs = collectKeyValuePairsUsingIterators<Base*, int>(weakHashMap);
EXPECT_EQ(pairs.size(), 0u);
- EXPECT_EQ(s_baseWeakReferences, 0u); // Iterating over WeakHashMap should have triggerd amortized deletion.
+ EXPECT_EQ(s_baseWeakReferences, 36u);
weakHashMap.checkConsistency();
- EXPECT_FALSE(weakHashMap.hasNullReferences());
+ EXPECT_TRUE(weakHashMap.hasNullReferences());
}
{
@@ -1744,8 +1744,8 @@
collectKeyValuePairsUsingIterators<Base*, int>(weakHashMap);
weakHashMap.checkConsistency();
- EXPECT_EQ(s_baseWeakReferences, 40u);
- EXPECT_FALSE(weakHashMap.hasNullReferences());
+ EXPECT_EQ(s_baseWeakReferences, 50u);
+ EXPECT_TRUE(weakHashMap.hasNullReferences());
for (unsigned i = 0; i < 50; ++i) {
if (!(i % 9))
@@ -1760,8 +1760,8 @@
collectKeyValuePairsUsingIterators<Base*, int>(weakHashMap);
weakHashMap.checkConsistency();
- EXPECT_EQ(s_baseWeakReferences, 36u);
- EXPECT_FALSE(weakHashMap.hasNullReferences());
+ EXPECT_EQ(s_baseWeakReferences, 40u);
+ EXPECT_TRUE(weakHashMap.hasNullReferences());
}
}
@@ -1797,9 +1797,9 @@
}
}
weakHashMap.checkConsistency();
- EXPECT_EQ(s_baseWeakReferences, 42u);
- EXPECT_EQ(ValueObject::s_count, 42u);
- EXPECT_FALSE(weakHashMap.hasNullReferences());
+ EXPECT_EQ(s_baseWeakReferences, 50u);
+ EXPECT_EQ(ValueObject::s_count, 50u);
+ EXPECT_TRUE(weakHashMap.hasNullReferences());
for (unsigned i = 0; i < 50; ++i) {
if (!(i % 3))
@@ -1839,6 +1839,30 @@
}
}
+TEST(WTF_WeakPtr, WeakHashMap_iterator_destruction)
+{
+ constexpr unsigned objectCount = 10;
+ WeakHashMap<Base, unsigned> weakHashMap;
+ Vector<std::unique_ptr<Base>> objects;
+ objects.reserveInitialCapacity(objectCount);
+ for (unsigned i = 0; i < objectCount; ++i) {
+ auto object = makeUnique<Base>();
+ weakHashMap.add(*object, i);
+ objects.uncheckedAppend(WTFMove(object));
+ }
+
+ auto a = objects.takeLast();
+ auto b = objects.takeLast();
+
+ auto aIterator = weakHashMap.find(*a);
+ objects.clear();
+ for (unsigned i = 0; i < 20; ++i) {
+ auto bIterator = weakHashMap.find(*b);
+ EXPECT_EQ(bIterator->value, objectCount - 2);
+ EXPECT_EQ(aIterator->value, objectCount - 1);
+ }
+}
+
class MultipleInheritanceBase1 : public CanMakeWeakPtr<MultipleInheritanceBase1> {
public:
virtual void meow() = 0;
