Branch: refs/heads/main
Home: https://github.com/WebKit/WebKit
Commit: 177d6881d88dfc4e1306156c513b7e9c81d41710
https://github.com/WebKit/WebKit/commit/177d6881d88dfc4e1306156c513b7e9c81d41710
Author: Chris Dumez <[email protected]>
Date: 2023-08-04 (Fri, 04 Aug 2023)
Changed paths:
M Source/WTF/wtf/ThreadSafeWeakHashSet.h
M Source/WTF/wtf/WeakHashMap.h
M Source/WTF/wtf/WeakHashSet.h
M Source/WTF/wtf/WeakListHashSet.h
M Source/WebKit/NetworkProcess/NetworkDataTask.cpp
M Source/WebKit/NetworkProcess/NetworkDataTaskBlob.cpp
M Source/WebKit/NetworkProcess/NetworkDataTaskDataURL.cpp
M Source/WebKit/NetworkProcess/NetworkSession.cpp
M Source/WebKit/NetworkProcess/NetworkSession.h
M Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm
M Source/WebKit/NetworkProcess/curl/NetworkDataTaskCurl.cpp
M Source/WebKit/NetworkProcess/soup/NetworkDataTaskSoup.cpp
M Tools/TestWebKitAPI/Tests/WTF/WeakPtr.cpp
Log Message:
-----------
Regression: NetworkDataTask's ThreadSafeWeakPtrControlBlock are leaking
https://bugs.webkit.org/show_bug.cgi?id=259808
rdar://112162921
Reviewed by Alex Christensen.
The NetworkProcess memory was growing unboundedly during long-running browsing
benchmarks, which the number of NetworkDataTask's ThreadSafeWeakPtrControlBlock
objects growing very large.
The issue was with the `ThreadSafeWeakHashSet<NetworkDataTask> m_dataTaskSet`
container on the NetworkSession. We would add NetworkDataTask objects to this
WeakHashSet and never explicitly remove them. We had a comment in the code
indicating that the ThreadSafeWeakHashSet's amortized clean up would take care
of removing them. The issue though is that amortized clean up would never happen
and m_dataTaskSet's internal Set would just grow unboundedly, keeping control
blocks alive.
The reason amortized clean up never triggered is that we only add to
m_dataTaskSet,
we never remove from it, never clear it and we only very rarely call forEach()
on
it. As a result, ThreadSafeWeakHashSet::m_operationCountSinceLastCleanup would
only
increment due to the add() calls. The condition for amortized clean up was:
`++m_operationCountSinceLastCleanup / 2 > m_set.size()`.
Since m_operationCountSinceLastCleanup would only increase due to the add()
calls
and since nothing would ever get removed from the set,
`m_operationCountSinceLastCleanup / 2`
could never reach the size of the set.
I am making several fixes in this patch:
1. I added a `|| m_operationCountSinceLastCleanup >
m_maxOperationCountWithoutCleanup`
check to amortized clean up to guarantee that amortized clean up eventually
happens
and that such weak containers can never grow unboundedly, no matter how they
are used.
m_maxOperationCountWithoutCleanup gets multiplied by 2 whenever the clean up
finds
nothing for proper amortization.
2. I made it so that NetworkDataTask objects remove themselves from the
ThreadSafeWeakHashSet
in their destructor. This is good practice no matter what and makes such the
set stays
small.
3. I actually found a bug in `ThreadSafeWeakHashSet::remove()` when
implementing the fix in
(2). ThreadSafeWeakHashSet::remove() would fail to remove the object from
the set if the
call if made after the object destructor has started running! The reason for
this is that
the ThreadSafeWeakHashSet was using a `std::pair<ControlBlockRefPtr, const
T*>` as set
key internally. This means that we needed to create a ControlBlockRefPtr of
the object's
control block in remove() in order to remove the object from the map.
However, 265344@main
made it so that ref'ing a control block returns nullptr after the object has
started
destruction. As a result, the first item in the key pair would be nullptr
and we'd fail
to remove. To address the issue, I am dropping the ControlBlockRefPtr from
the key and
using a `HashMap<const T*, ControlBlockRefPtr>` instead of a HashSet.
* Source/WTF/wtf/ThreadSafeWeakHashSet.h:
* Source/WTF/wtf/WeakHashMap.h:
* Source/WTF/wtf/WeakHashSet.h:
* Source/WTF/wtf/WeakListHashSet.h:
* Source/WebKit/NetworkProcess/NetworkDataTask.cpp:
(WebKit::NetworkDataTask::~NetworkDataTask):
* Source/WebKit/NetworkProcess/NetworkSession.cpp:
(WebKit::NetworkSession::registerNetworkDataTask):
(WebKit::NetworkSession::unregisterNetworkDataTask):
* Source/WebKit/NetworkProcess/NetworkSession.h:
* Tools/TestWebKitAPI/Tests/WTF/WeakPtr.cpp:
(TestWebKitAPI::ObjectAddingAndRemovingItself::create):
(TestWebKitAPI::ObjectAddingAndRemovingItself::~ObjectAddingAndRemovingItself):
(TestWebKitAPI::ObjectAddingAndRemovingItself::ObjectAddingAndRemovingItself):
(TestWebKitAPI::TEST):
(TestWebKitAPI::ObjectAddingItselfOnly::create):
(TestWebKitAPI::ObjectAddingItselfOnly::ObjectAddingItselfOnly):
Canonical link: https://commits.webkit.org/266594@main
_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes
