Attachment 369323 Details for Bug 197666 – Patch

[patch] Patch

bug-197666-20190507133345.patch (text/plain), 12.73 KB, created by Chris Dumez on 2019-05-07 13:33:46 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Chris Dumez

Created: 2019-05-07 13:33:46 PDT

Size: 12.73 KB

patch

obsolete

>Subversion Revision: 245028
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index f4882f86c968dcf40d2b0d82dff3d57e3d741841..c49630305d29b771ba3c4c73fc4bd1fc9df11b7b 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,17 @@
>+2019-05-07  Chris Dumez  <cdumez@apple.com>
>+
>+        Regression: Crash at WebKit: PAL::HysteresisActivity::start
>+        https://bugs.webkit.org/show_bug.cgi?id=197666
>+        <rdar://problem/50037153>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Add utility function to query if an object is the current SQLiteDatabaseTrackerClient.
>+
>+        * platform/sql/SQLiteDatabaseTracker.cpp:
>+        (WebCore::SQLiteDatabaseTracker::isCurrentClient):
>+        * platform/sql/SQLiteDatabaseTracker.h:
>+
> 2019-05-07  John Wilander  <wilander@apple.com>
> 
>         Storage Access API: Make two changes requested by developers and complete refactoring and cleanup
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index 8f8a1d25ed49a4a8faa0c0302220d21fb1e611f1..fe74a16f6cf2a7442f4ed2ab110a11df05e04b83 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,44 @@
>+2019-05-07  Chris Dumez  <cdumez@apple.com>
>+
>+        Regression: Crash at WebKit: PAL::HysteresisActivity::start
>+        https://bugs.webkit.org/show_bug.cgi?id=197666
>+        <rdar://problem/50037153>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        We've recently made it so that the WebContent process destroys its WebSQLiteDatabaseTracker when preparing
>+        for process suspension and then re-constructs it when resuming. The issue is that the WebSQLiteDatabaseTracker
>+        internal implementation was calling callOnMainThread() and capturing |this| to start/stop its HysteresisActivity.
>+        As a result, |this| could be dead by the time we're on the main thread and we'd crash.
>+
>+        To address the issue, update WebSQLiteDatabaseTracker::willBeginFirstTransaction() / didFinishLastTransaction()
>+        to check if |this| is still the current SQLiteDatabaseTracker client on the main thread, and abort early if
>+        it isn't. This makes sure we do not use |this| after it is dead since the WebSQLiteDatabaseTracker destructor
>+        unsets itself as a SQLiteDatabaseTracker client.
>+
>+        Also clean up the class a bit so that:
>+        1. The constructor takes in a WTF::Function instead of a NetworkProcess / WebProcess crash. This is provides
>+           better layering. The WebSQLiteDatabaseTracker should not need to know anything about those objects.
>+        2. Use RunLoop::main().dispatch() instead of callOnMainThread() since we're in WebKit2 code.
>+        3. Mark class as fast-allocated and final.
>+
>+        * NetworkProcess/NetworkProcess.cpp:
>+        (WebKit::NetworkProcess::NetworkProcess):
>+        * Shared/WebSQLiteDatabaseTracker.cpp:
>+        (WebKit::WebSQLiteDatabaseTracker::WebSQLiteDatabaseTracker):
>+        (WebKit::WebSQLiteDatabaseTracker::~WebSQLiteDatabaseTracker):
>+        (WebKit::WebSQLiteDatabaseTracker::willBeginFirstTransaction):
>+        (WebKit::WebSQLiteDatabaseTracker::didFinishLastTransaction):
>+        (WebKit::WebSQLiteDatabaseTracker::hysteresisUpdated): Deleted.
>+        * Shared/WebSQLiteDatabaseTracker.h:
>+        * WebProcess/WebProcess.cpp:
>+        (WebKit::m_nonVisibleProcessCleanupTimer):
>+        (WebKit::WebProcess::initializeSQLiteDatabaseTracker):
>+        (WebKit::WebProcess::cancelPrepareToSuspend):
>+        (WebKit::WebProcess::processDidResume):
>+        (WebKit::m_webSQLiteDatabaseTracker): Deleted.
>+        * WebProcess/WebProcess.h:
>+
> 2019-05-07  Adrian Perez de Castro  <aperez@igalia.com>
> 
>         [GTK][WPE] Cannot build documentation with gtk-doc
>diff --git a/Source/WebCore/platform/sql/SQLiteDatabaseTracker.cpp b/Source/WebCore/platform/sql/SQLiteDatabaseTracker.cpp
>index 4af290759b7e825d4230024f6db9e0a16c64161b..e1e285c632fc357de86e8ef8f1ce6e85c6ac02f3 100644
>--- a/Source/WebCore/platform/sql/SQLiteDatabaseTracker.cpp
>+++ b/Source/WebCore/platform/sql/SQLiteDatabaseTracker.cpp
>@@ -44,6 +44,12 @@ void setClient(SQLiteDatabaseTrackerClient* client)
>     s_staticSQLiteDatabaseTrackerClient = client;
> }
> 
>+bool isCurrentClient(SQLiteDatabaseTrackerClient* client)
>+{
>+    std::lock_guard<Lock> lock(transactionInProgressMutex);
>+    return s_staticSQLiteDatabaseTrackerClient == client;
>+}
>+
> void incrementTransactionInProgressCount()
> {
>     std::lock_guard<Lock> lock(transactionInProgressMutex);
>diff --git a/Source/WebCore/platform/sql/SQLiteDatabaseTracker.h b/Source/WebCore/platform/sql/SQLiteDatabaseTracker.h
>index 0e9ffbbfa0169ff5ddf1bbe9770e876a07550462..4d1afb806660ed1dcaaf923d0cfe7f525216f473 100644
>--- a/Source/WebCore/platform/sql/SQLiteDatabaseTracker.h
>+++ b/Source/WebCore/platform/sql/SQLiteDatabaseTracker.h
>@@ -36,6 +36,7 @@ WEBCORE_EXPORT void decrementTransactionInProgressCount();
> WEBCORE_EXPORT void incrementTransactionInProgressCount();
> 
> WEBCORE_EXPORT void setClient(SQLiteDatabaseTrackerClient*);
>+WEBCORE_EXPORT bool isCurrentClient(SQLiteDatabaseTrackerClient*);
> 
> WEBCORE_EXPORT bool hasTransactionInProgress();
> 
>diff --git a/Source/WebKit/NetworkProcess/NetworkProcess.cpp b/Source/WebKit/NetworkProcess/NetworkProcess.cpp
>index 0e7180db2bcf410902d449c7c26388ad5f46e878..6dd5c35ee6d094c9f63e20915ed8089de065a460 100644
>--- a/Source/WebKit/NetworkProcess/NetworkProcess.cpp
>+++ b/Source/WebKit/NetworkProcess/NetworkProcess.cpp
>@@ -133,7 +133,7 @@ NetworkProcess::NetworkProcess(AuxiliaryProcessInitializationParameters&& parame
>     , m_networkContentRuleListManager(*this)
> #endif
> #if PLATFORM(IOS_FAMILY)
>-    , m_webSQLiteDatabaseTracker(*this)
>+    , m_webSQLiteDatabaseTracker([this](bool isHoldingLockedFiles) { parentProcessConnection()->send(Messages::NetworkProcessProxy::SetIsHoldingLockedFiles(isHoldingLockedFiles), 0); })
> #endif
> {
>     NetworkProcessPlatformStrategies::initialize();
>diff --git a/Source/WebKit/Shared/WebSQLiteDatabaseTracker.cpp b/Source/WebKit/Shared/WebSQLiteDatabaseTracker.cpp
>index cc69f5e5333e91a7303f746bace834544f4c8c68..14ca331578e651c230d4925b82fe9519454b564e 100644
>--- a/Source/WebKit/Shared/WebSQLiteDatabaseTracker.cpp
>+++ b/Source/WebKit/Shared/WebSQLiteDatabaseTracker.cpp
>@@ -31,24 +31,15 @@
> #include "WebProcess.h"
> #include "WebProcessProxyMessages.h"
> #include <WebCore/SQLiteDatabaseTracker.h>
>-#include <wtf/MainThread.h>
> 
> namespace WebKit {
> using namespace WebCore;
> 
>-WebSQLiteDatabaseTracker::WebSQLiteDatabaseTracker(WebProcess& process)
>-    : m_process(process)
>-    , m_processType(AuxiliaryProcess::ProcessType::WebContent)
>-    , m_hysteresis([this](PAL::HysteresisState state) { hysteresisUpdated(state); })
>-{
>-    SQLiteDatabaseTracker::setClient(this);
>-}
>-
>-WebSQLiteDatabaseTracker::WebSQLiteDatabaseTracker(NetworkProcess& process)
>-    : m_process(process)
>-    , m_processType(AuxiliaryProcess::ProcessType::Network)
>-    , m_hysteresis([this](PAL::HysteresisState state) { hysteresisUpdated(state); })
>+WebSQLiteDatabaseTracker::WebSQLiteDatabaseTracker(IsHoldingLockedFilesHandler&& isHoldingLockedFilesHandler)
>+    : m_isHoldingLockedFilesHandler(WTFMove(isHoldingLockedFilesHandler))
>+    , m_hysteresis([this](PAL::HysteresisState state) { m_isHoldingLockedFilesHandler(state == PAL::HysteresisState::Started); })
> {
>+    ASSERT(RunLoop::isMain());
>     SQLiteDatabaseTracker::setClient(this);
> }
> 
>@@ -58,35 +49,27 @@ WebSQLiteDatabaseTracker::~WebSQLiteDatabaseTracker()
>     SQLiteDatabaseTracker::setClient(nullptr);
> 
>     if (m_hysteresis.state() == PAL::HysteresisState::Started)
>-        hysteresisUpdated(PAL::HysteresisState::Stopped);
>+        m_isHoldingLockedFilesHandler(false);
> }
> 
> void WebSQLiteDatabaseTracker::willBeginFirstTransaction()
> {
>-    callOnMainThread([this] {
>+    RunLoop::main().dispatch([this] {
>+        if (!SQLiteDatabaseTracker::isCurrentClient(this))
>+            return;
>+
>         m_hysteresis.start();
>     });
> }
> 
> void WebSQLiteDatabaseTracker::didFinishLastTransaction()
> {
>-    callOnMainThread([this] {
>+    RunLoop::main().dispatch([this] {
>+        if (!SQLiteDatabaseTracker::isCurrentClient(this))
>+            return;
>+
>         m_hysteresis.stop();
>     });
> }
> 
>-void WebSQLiteDatabaseTracker::hysteresisUpdated(PAL::HysteresisState state)
>-{
>-    switch (m_processType) {
>-    case AuxiliaryProcess::ProcessType::WebContent:
>-        m_process.parentProcessConnection()->send(Messages::WebProcessProxy::SetIsHoldingLockedFiles(state == PAL::HysteresisState::Started), 0);
>-        break;
>-    case AuxiliaryProcess::ProcessType::Network:
>-        m_process.parentProcessConnection()->send(Messages::NetworkProcessProxy::SetIsHoldingLockedFiles(state == PAL::HysteresisState::Started), 0);
>-        break;
>-    default:
>-        ASSERT_NOT_REACHED();
>-    }
>-}
>-
> } // namespace WebKit
>diff --git a/Source/WebKit/Shared/WebSQLiteDatabaseTracker.h b/Source/WebKit/Shared/WebSQLiteDatabaseTracker.h
>index 0f6523291abc1775b33169be42e032f809e43dae..119ef492c3d02c74fbd5c38d688897fa3a46712e 100644
>--- a/Source/WebKit/Shared/WebSQLiteDatabaseTracker.h
>+++ b/Source/WebKit/Shared/WebSQLiteDatabaseTracker.h
>@@ -31,26 +31,21 @@
> 
> namespace WebKit {
> 
>-class NetworkProcess;
>-class WebProcess;
>-
>-class WebSQLiteDatabaseTracker : public WebCore::SQLiteDatabaseTrackerClient {
>+class WebSQLiteDatabaseTracker final: public WebCore::SQLiteDatabaseTrackerClient {
>     WTF_MAKE_NONCOPYABLE(WebSQLiteDatabaseTracker)
>+    WTF_MAKE_FAST_ALLOCATED;
> public:
>-    explicit WebSQLiteDatabaseTracker(WebProcess&);
>-    explicit WebSQLiteDatabaseTracker(NetworkProcess&);
>+    using IsHoldingLockedFilesHandler = Function<void(bool)>;
>+    explicit WebSQLiteDatabaseTracker(IsHoldingLockedFilesHandler&&);
> 
>     ~WebSQLiteDatabaseTracker();
> 
>-    // WebCore::SQLiteDatabaseTrackerClient
>-    void willBeginFirstTransaction() override;
>-    void didFinishLastTransaction() override;
>-
> private:
>-    void hysteresisUpdated(PAL::HysteresisState);
>+    // WebCore::SQLiteDatabaseTrackerClient.
>+    void willBeginFirstTransaction() final;
>+    void didFinishLastTransaction() final;
> 
>-    AuxiliaryProcess& m_process;
>-    AuxiliaryProcess::ProcessType m_processType;
>+    IsHoldingLockedFilesHandler m_isHoldingLockedFilesHandler;
>     PAL::HysteresisActivity m_hysteresis;
> };
> 
>diff --git a/Source/WebKit/WebProcess/WebProcess.cpp b/Source/WebKit/WebProcess/WebProcess.cpp
>index 6fa34f0e1ed1754a6df0193f3cee72cfc3438cb2..a939af17aa46ab88ce99d9fce48d5d8464edacd1 100644
>--- a/Source/WebKit/WebProcess/WebProcess.cpp
>+++ b/Source/WebKit/WebProcess/WebProcess.cpp
>@@ -187,10 +187,9 @@ WebProcess::WebProcess()
>     , m_pluginProcessConnectionManager(PluginProcessConnectionManager::create())
> #endif
>     , m_nonVisibleProcessCleanupTimer(*this, &WebProcess::nonVisibleProcessCleanupTimerFired)
>-#if PLATFORM(IOS_FAMILY)
>-    , m_webSQLiteDatabaseTracker(std::make_unique<WebSQLiteDatabaseTracker>(*this))
>-#endif
> {
>+    initializeSQLiteDatabaseTracker();
>+
>     // Initialize our platform strategies.
>     WebPlatformStrategies::initialize();
> 
>@@ -735,6 +734,15 @@ void WebProcess::didReceiveSyncMessage(IPC::Connection& connection, IPC::Decoder
>     didReceiveSyncWebProcessMessage(connection, decoder, replyEncoder);
> }
> 
>+void WebProcess::initializeSQLiteDatabaseTracker()
>+{
>+#if PLATFORM(IOS_FAMILY)
>+    m_webSQLiteDatabaseTracker = std::make_unique<WebSQLiteDatabaseTracker>([this](bool isHoldingLockedFiles) {
>+        parentProcessConnection()->send(Messages::WebProcessProxy::SetIsHoldingLockedFiles(isHoldingLockedFiles), 0);
>+    });
>+#endif
>+}
>+
> void WebProcess::didReceiveMessage(IPC::Connection& connection, IPC::Decoder& decoder)
> {
>     if (messageReceiverMap().dispatchMessage(connection, decoder))
>@@ -1519,7 +1527,7 @@ void WebProcess::cancelPrepareToSuspend()
>     unfreezeAllLayerTrees();
> 
> #if PLATFORM(IOS_FAMILY)
>-    m_webSQLiteDatabaseTracker = std::make_unique<WebSQLiteDatabaseTracker>(*this);
>+    initializeSQLiteDatabaseTracker();
>     SQLiteDatabase::setIsDatabaseOpeningForbidden(false);
>     accessibilityProcessSuspendedNotification(false);
> #endif
>@@ -1595,7 +1603,7 @@ void WebProcess::processDidResume()
>     unfreezeAllLayerTrees();
>     
> #if PLATFORM(IOS_FAMILY)
>-    m_webSQLiteDatabaseTracker = std::make_unique<WebSQLiteDatabaseTracker>(*this);
>+    initializeSQLiteDatabaseTracker();
>     SQLiteDatabase::setIsDatabaseOpeningForbidden(false);
>     accessibilityProcessSuspendedNotification(false);
> #endif
>diff --git a/Source/WebKit/WebProcess/WebProcess.h b/Source/WebKit/WebProcess/WebProcess.h
>index 09d6c5077ccf6b13fe0111de31252e068cc196de..82efd1536c05849968bb2d0a3a2ddfd6c81a2b62 100644
>--- a/Source/WebKit/WebProcess/WebProcess.h
>+++ b/Source/WebKit/WebProcess/WebProcess.h
>@@ -448,6 +448,8 @@ private:
>     void resumeAllMediaBuffering();
> #endif
> 
>+    void initializeSQLiteDatabaseTracker();
>+
>     void clearCurrentModifierStateForTesting();
> 
>     RefPtr<WebConnectionToUIProcess> m_webConnection;

Actions: View | Formatted Diff | Diff

Attachments on bug 197666: 369302 | 369307 | 369323 | 369343 | 369348 | 369349 | 369411