Attachment 369348 Details for Bug 197666 – Patch

[patch] Patch

bug-197666-20190507183604.patch (text/plain), 10.99 KB, created by Chris Dumez on 2019-05-07 18:36:05 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Chris Dumez

Created: 2019-05-07 18:36:05 PDT

Size: 10.99 KB

patch

obsolete

>Subversion Revision: 245046
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index 009e11ef825da4efafa48366036159f9724dd768..834bcc332b68a97bc9e188697b5f8c961a870305 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,41 @@
>+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, we no longer destroy the WebSQLiteDatabaseTracker when preparing to suspend. Instead, we
>+        set a 'isSuspended' flag on the WebSQLiteDatabaseTracker so that it stops notifying the WebProcess of changes.
>+
>+        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.
>+
>+        * 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  Carlos Garcia Campos  <cgarcia@igalia.com>
> 
>         REGRESSION(r244230): Web Automation: use null instead of empty string as success result of scripts when alert is open
>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..526e702d785d336b0a8cdd882058052883004488 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) { notifyClient(state == PAL::HysteresisState::Started); })
> {
>+    ASSERT(RunLoop::isMain());
>     SQLiteDatabaseTracker::setClient(this);
> }
> 
>@@ -58,35 +49,37 @@ WebSQLiteDatabaseTracker::~WebSQLiteDatabaseTracker()
>     SQLiteDatabaseTracker::setClient(nullptr);
> 
>     if (m_hysteresis.state() == PAL::HysteresisState::Started)
>-        hysteresisUpdated(PAL::HysteresisState::Stopped);
>+        notifyClient(false);
>+}
>+
>+bool WebSQLiteDatabaseTracker::setIsSuspended(bool isSuspended)
>+{
>+    ASSERT(RunLoop::isMain());
>+    m_isSuspended = isSuspended;
> }
> 
> void WebSQLiteDatabaseTracker::willBeginFirstTransaction()
> {
>-    callOnMainThread([this] {
>+    RunLoop::main().dispatch([this] {
>         m_hysteresis.start();
>     });
> }
> 
> void WebSQLiteDatabaseTracker::didFinishLastTransaction()
> {
>-    callOnMainThread([this] {
>+    RunLoop::main().dispatch([this] {
>         m_hysteresis.stop();
>     });
> }
> 
>-void WebSQLiteDatabaseTracker::hysteresisUpdated(PAL::HysteresisState state)
>+void WebSQLiteDatabaseTracker::notifyClient(bool isHoldingLockedFiles)
> {
>-    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();
>-    }
>+    ASSERT(RunLoop::isMain());
>+
>+    if (m_isSuspended)
>+        return;
>+
>+    m_isHoldingLockedFilesHandler(isHoldingLockedFiles);
> }
> 
> } // namespace WebKit
>diff --git a/Source/WebKit/Shared/WebSQLiteDatabaseTracker.h b/Source/WebKit/Shared/WebSQLiteDatabaseTracker.h
>index 0f6523291abc1775b33169be42e032f809e43dae..31b6f3ea2e55d952c9db04678c9d65cbf4c2f546 100644
>--- a/Source/WebKit/Shared/WebSQLiteDatabaseTracker.h
>+++ b/Source/WebKit/Shared/WebSQLiteDatabaseTracker.h
>@@ -31,27 +31,26 @@
> 
> namespace WebKit {
> 
>-class NetworkProcess;
>-class WebProcess;
>-
>-class WebSQLiteDatabaseTracker : public WebCore::SQLiteDatabaseTrackerClient {
>+class WebSQLiteDatabaseTracker final : public WebCore::SQLiteDatabaseTrackerClient {
>     WTF_MAKE_NONCOPYABLE(WebSQLiteDatabaseTracker)
> public:
>-    explicit WebSQLiteDatabaseTracker(WebProcess&);
>-    explicit WebSQLiteDatabaseTracker(NetworkProcess&);
>+    using IsHoldingLockedFilesHandler = Function<void(bool)>;
>+    explicit WebSQLiteDatabaseTracker(IsHoldingLockedFilesHandler&&);
> 
>     ~WebSQLiteDatabaseTracker();
> 
>-    // WebCore::SQLiteDatabaseTrackerClient
>-    void willBeginFirstTransaction() override;
>-    void didFinishLastTransaction() override;
>+    bool setIsSuspended(bool);
> 
> private:
>-    void hysteresisUpdated(PAL::HysteresisState);
>+    void notifyClient(bool isHoldingLockedFiles);
>+
>+    // WebCore::SQLiteDatabaseTrackerClient.
>+    void willBeginFirstTransaction() final;
>+    void didFinishLastTransaction() final;
> 
>-    AuxiliaryProcess& m_process;
>-    AuxiliaryProcess::ProcessType m_processType;
>+    IsHoldingLockedFilesHandler m_isHoldingLockedFilesHandler;
>     PAL::HysteresisActivity m_hysteresis;
>+    bool m_isSuspended { false };
> };
> 
> } // namespace WebKit
>diff --git a/Source/WebKit/WebProcess/WebProcess.cpp b/Source/WebKit/WebProcess/WebProcess.cpp
>index 6fa34f0e1ed1754a6df0193f3cee72cfc3438cb2..2528387ab3b7a226828030be75252d41ca838a0a 100644
>--- a/Source/WebKit/WebProcess/WebProcess.cpp
>+++ b/Source/WebKit/WebProcess/WebProcess.cpp
>@@ -188,7 +188,7 @@ WebProcess::WebProcess()
> #endif
>     , m_nonVisibleProcessCleanupTimer(*this, &WebProcess::nonVisibleProcessCleanupTimerFired)
> #if PLATFORM(IOS_FAMILY)
>-    , m_webSQLiteDatabaseTracker(std::make_unique<WebSQLiteDatabaseTracker>(*this))
>+    , m_webSQLiteDatabaseTracker([this](bool isHoldingLockedFiles) { parentProcessConnection()->send(Messages::WebProcessProxy::SetIsHoldingLockedFiles(isHoldingLockedFiles), 0); })
> #endif
> {
>     // Initialize our platform strategies.
>@@ -1468,7 +1468,7 @@ void WebProcess::actualPrepareToSuspend(ShouldAcknowledgeWhenReadyToSuspend shou
> #endif
> 
> #if PLATFORM(IOS_FAMILY)
>-    m_webSQLiteDatabaseTracker = nullptr;
>+    m_webSQLiteDatabaseTracker.setIsSuspended(true);
>     SQLiteDatabase::setIsDatabaseOpeningForbidden(true);
>     if (DatabaseTracker::isInitialized())
>         DatabaseTracker::singleton().closeAllDatabases(CurrentQueryBehavior::Interrupt);
>@@ -1519,7 +1519,7 @@ void WebProcess::cancelPrepareToSuspend()
>     unfreezeAllLayerTrees();
> 
> #if PLATFORM(IOS_FAMILY)
>-    m_webSQLiteDatabaseTracker = std::make_unique<WebSQLiteDatabaseTracker>(*this);
>+    m_webSQLiteDatabaseTracker.setIsSuspended(false);
>     SQLiteDatabase::setIsDatabaseOpeningForbidden(false);
>     accessibilityProcessSuspendedNotification(false);
> #endif
>@@ -1595,7 +1595,7 @@ void WebProcess::processDidResume()
>     unfreezeAllLayerTrees();
>     
> #if PLATFORM(IOS_FAMILY)
>-    m_webSQLiteDatabaseTracker = std::make_unique<WebSQLiteDatabaseTracker>(*this);
>+    m_webSQLiteDatabaseTracker.setIsSuspended(false);
>     SQLiteDatabase::setIsDatabaseOpeningForbidden(false);
>     accessibilityProcessSuspendedNotification(false);
> #endif
>diff --git a/Source/WebKit/WebProcess/WebProcess.h b/Source/WebKit/WebProcess/WebProcess.h
>index 09d6c5077ccf6b13fe0111de31252e068cc196de..30bd59a715297470d3e2932959957e6d4e2a60d4 100644
>--- a/Source/WebKit/WebProcess/WebProcess.h
>+++ b/Source/WebKit/WebProcess/WebProcess.h
>@@ -37,6 +37,7 @@
> #include "ViewUpdateDispatcher.h"
> #include "WebInspectorInterruptDispatcher.h"
> #include "WebProcessCreationParameters.h"
>+#include "WebSQLiteDatabaseTracker.h"
> #include <WebCore/ActivityState.h>
> #include <WebCore/RegistrableDomain.h>
> #if PLATFORM(MAC)
>@@ -112,7 +113,6 @@ enum class WebsiteDataType;
> struct WebPageCreationParameters;
> struct WebPageGroupData;
> struct WebPreferencesStore;
>-class WebSQLiteDatabaseTracker;
> struct WebsiteData;
> struct WebsiteDataStoreParameters;
> 
>@@ -512,7 +512,7 @@ private:
>     RefPtr<WebCore::ApplicationCacheStorage> m_applicationCacheStorage;
> 
> #if PLATFORM(IOS_FAMILY)
>-    std::unique_ptr<WebSQLiteDatabaseTracker> m_webSQLiteDatabaseTracker;
>+    WebSQLiteDatabaseTracker m_webSQLiteDatabaseTracker;
>     ProcessTaskStateObserver m_taskStateObserver { *this };
> #endif
> #if HAVE(VISIBILITY_PROPAGATION_VIEW)

Actions: View | Formatted Diff | Diff

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