Changeset 247094 in webkit

Timestamp:

Jul 3, 2019 10:48:41 AM (5 years ago)

Author:

Chris Dumez

Message:

Crash under WTF::RefCounted<WebKit::TaskCounter>::deref()
​https://bugs.webkit.org/show_bug.cgi?id=199453
<rdar://problem/51991477>

Reviewed by Youenn Fablet.

The crash was caused by StorageManager::suspend() getting called on the main thread but calling
its completion handler on a background queue. The completion handler was capturing a TaskCounter
object which is RefCounted (not ThreadSafeRefCounted).

Address the issue by making sure StorageManager::suspend() calls its completion handler on the
main thread. Also get rid of TaskCounter and use a WTF::CallbackAggregator instead.

• NetworkProcess/NetworkProcess.cpp:

(WebKit::NetworkProcess::actualPrepareToSuspend):
(WebKit::TaskCounter::TaskCounter): Deleted.
(WebKit::TaskCounter::~TaskCounter): Deleted.

• NetworkProcess/WebStorage/StorageManager.cpp:

(WebKit::StorageManager::suspend):

Location:

trunk/Source/WebKit

Files:

• ChangeLog (modified) (1 diff)
• NetworkProcess/NetworkProcess.cpp (modified) (2 diffs)
• NetworkProcess/WebStorage/StorageManager.cpp (modified) (1 diff)

trunk/Source/WebKit/ChangeLog

@@ +1 @@
+2019-07-03  Chris Dumez  <cdumez@apple.com>
+
+        Crash under WTF::RefCounted<WebKit::TaskCounter>::deref()
+        https://bugs.webkit.org/show_bug.cgi?id=199453
+        <rdar://problem/51991477>
+
+        Reviewed by Youenn Fablet.
+
+        The crash was caused by StorageManager::suspend() getting called on the main thread but calling
+        its completion handler on a background queue. The completion handler was capturing a TaskCounter
+        object which is RefCounted (not ThreadSafeRefCounted).
+
+        Address the issue by making sure StorageManager::suspend() calls its completion handler on the
+        main thread. Also get rid of TaskCounter and use a WTF::CallbackAggregator instead.
+
+        * NetworkProcess/NetworkProcess.cpp:
+        (WebKit::NetworkProcess::actualPrepareToSuspend):
+        (WebKit::TaskCounter::TaskCounter): Deleted.
+        (WebKit::TaskCounter::~TaskCounter): Deleted.
+        * NetworkProcess/WebStorage/StorageManager.cpp:
+        (WebKit::StorageManager::suspend):
+
 2019-07-03  Youenn Fablet  <youenn@apple.com>
 

trunk/Source/WebKit/NetworkProcess/NetworkProcess.cpp

@@ -2030 +2030 @@
 }
 
-// FIXME: We can remove this one by adapting RefCounter.
-class TaskCounter : public RefCounted<TaskCounter> {
-public:
-    explicit TaskCounter(Function<void()>&& callback) : m_callback(WTFMove(callback)) { }
-    ~TaskCounter() { m_callback(); };
-
-private:
-    Function<void()> m_callback;
-};
-
 void NetworkProcess::actualPrepareToSuspend(ShouldAcknowledgeWhenReadyToSuspend shouldAcknowledgeWhenReadyToSuspend)
 {
@@ -2048 +2038 @@
     lowMemoryHandler(Critical::Yes);
 
-    RefPtr<TaskCounter> delayedTaskCounter;
+    RefPtr<CallbackAggregator> callbackAggregator;
     if (shouldAcknowledgeWhenReadyToSuspend == ShouldAcknowledgeWhenReadyToSuspend::Yes) {
-        delayedTaskCounter = adoptRef(new TaskCounter([this] {
+        callbackAggregator = CallbackAggregator::create([this] {
             RELEASE_LOG(ProcessSuspension, "%p - NetworkProcess::notifyProcessReadyToSuspend() Sending ProcessReadyToSuspend IPC message", this);
             if (parentProcessConnection())
                 parentProcessConnection()->send(Messages::NetworkProcessProxy::ProcessReadyToSuspend(), 0);
-        }));
-    }
-
-    platformPrepareToSuspend([delayedTaskCounter] { });
-    platformSyncAllCookies([delayedTaskCounter] { });
+        });
+    }
+
+    platformPrepareToSuspend([callbackAggregator] { });
+    platformSyncAllCookies([callbackAggregator] { });
 
     for (auto& connection : m_webProcessConnections)
-        connection->cleanupForSuspension([delayedTaskCounter] { });
+        connection->cleanupForSuspension([callbackAggregator] { });
 
 #if ENABLE(SERVICE_WORKER)
     for (auto& server : m_swServers.values()) {
         ASSERT(m_swServers.get(server->sessionID()) == server.get());
-        server->startSuspension([delayedTaskCounter] { });
+        server->startSuspension([callbackAggregator] { });
     }
 #endif
 
     for (auto& session : m_networkSessions)
-        session.value->storageManager().suspend([delayedTaskCounter] { });
+        session.value->storageManager().suspend([callbackAggregator] { });
 }
 

trunk/Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp

@@ -937 +937 @@
         ASSERT(m_state != State::Suspended);
 
-        completionHandler();
-
-        if (m_state != State::WillSuspend)
+        if (m_state != State::WillSuspend) {
+            RunLoop::main().dispatch(WTFMove(completionHandler));
             return;
+        }
+
         m_state = State::Suspended;
+        RunLoop::main().dispatch(WTFMove(completionHandler));
+        
         while (m_state == State::Suspended)
             m_stateChangeCondition.wait(m_stateLock);