Attachment 373395 Details for Bug 199453 – Patch

[patch] Patch

bug-199453-20190703100447.patch (text/plain), 4.99 KB, created by Chris Dumez on 2019-07-03 10:04:48 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Chris Dumez

Created: 2019-07-03 10:04:48 PDT

Size: 4.99 KB

patch

obsolete

>Subversion Revision: 247091
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index 4c9425a61dc96b68e8d2d881dcc6dfe02963bbf0..78150783208f328392645c5055c8a91f0decd3a8 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,25 @@
>+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 NOBODY (OOPS!).
>+
>+        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-02  Joonghun Park  <jh718.park@samsung.com>
> 
>         Unreviewed. Fix build break introduced in r247058.
>diff --git a/Source/WebKit/NetworkProcess/NetworkProcess.cpp b/Source/WebKit/NetworkProcess/NetworkProcess.cpp
>index 12c21dedf1aab33c10c25054ceae9b2a1ea10f63..e8e15ca087fc8210644716bbded60333470d87f5 100644
>--- a/Source/WebKit/NetworkProcess/NetworkProcess.cpp
>+++ b/Source/WebKit/NetworkProcess/NetworkProcess.cpp
>@@ -2029,16 +2029,6 @@ void NetworkProcess::processDidTransitionToBackground()
>     platformProcessDidTransitionToBackground();
> }
> 
>-// 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)
> {
> #if PLATFORM(IOS_FAMILY)
>@@ -2047,30 +2037,30 @@ void NetworkProcess::actualPrepareToSuspend(ShouldAcknowledgeWhenReadyToSuspend
> 
>     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] { });
> }
> 
> void NetworkProcess::processWillSuspendImminently()
>diff --git a/Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp b/Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp
>index 76c0fe53928e377877b15a11b0c0ce929f5833eb..6413d0c977d671232622fb880800faadcc659be2 100644
>--- a/Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp
>+++ b/Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp
>@@ -948,11 +948,14 @@ void StorageManager::suspend(CompletionHandler<void()>&& completionHandler)
>         Locker<Lock> stateLocker(m_stateLock);
>         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);
>         ASSERT(m_state == State::Running);

Actions: View | Formatted Diff | Diff

Attachments on bug 199453: 373395