[webkit-changes] [WebKit/WebKit] 177d68: Regression: NetworkDataTask's ThreadSafeWeakPtrCon...

[Chris Dumez]

  Branch: refs/heads/main
  Home:   https://github.com/WebKit/WebKit
  Commit: 177d6881d88dfc4e1306156c513b7e9c81d41710
      https://github.com/WebKit/WebKit/commit/177d6881d88dfc4e1306156c513b7e9c81d41710
  Author: Chris Dumez <cdumez at apple.com>
  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 at 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