- Revision
- 245069
- Author
- [email protected]
- Date
- 2019-05-08 14:18:48 -0700 (Wed, 08 May 2019)
Log Message
Regression: Crash at WebKit: PAL::HysteresisActivity::start
https://bugs.webkit.org/show_bug.cgi?id=197666
<rdar://problem/50037153>
Reviewed by Geoffrey Garen.
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 reference. 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:
Modified Paths
Diff
Modified: trunk/Source/WebKit/ChangeLog (245068 => 245069)
--- trunk/Source/WebKit/ChangeLog 2019-05-08 21:14:39 UTC (rev 245068)
+++ trunk/Source/WebKit/ChangeLog 2019-05-08 21:18:48 UTC (rev 245069)
@@ -1,3 +1,41 @@
+2019-05-08 Chris Dumez <[email protected]>
+
+ Regression: Crash at WebKit: PAL::HysteresisActivity::start
+ https://bugs.webkit.org/show_bug.cgi?id=197666
+ <rdar://problem/50037153>
+
+ Reviewed by Geoffrey Garen.
+
+ 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 reference. 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-08 Tim Horton <[email protected]>
iOS: Selection is dismissed even if click is preventDefault()'d
Modified: trunk/Source/WebKit/NetworkProcess/NetworkProcess.cpp (245068 => 245069)
--- trunk/Source/WebKit/NetworkProcess/NetworkProcess.cpp 2019-05-08 21:14:39 UTC (rev 245068)
+++ trunk/Source/WebKit/NetworkProcess/NetworkProcess.cpp 2019-05-08 21:18:48 UTC (rev 245069)
@@ -133,7 +133,7 @@
, 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();
Modified: trunk/Source/WebKit/Shared/WebSQLiteDatabaseTracker.cpp (245068 => 245069)
--- trunk/Source/WebKit/Shared/WebSQLiteDatabaseTracker.cpp 2019-05-08 21:14:39 UTC (rev 245068)
+++ trunk/Source/WebKit/Shared/WebSQLiteDatabaseTracker.cpp 2019-05-08 21:18:48 UTC (rev 245069)
@@ -31,27 +31,18 @@
#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); })
+WebSQLiteDatabaseTracker::WebSQLiteDatabaseTracker(IsHoldingLockedFilesHandler&& isHoldingLockedFilesHandler)
+ : m_isHoldingLockedFilesHandler(WTFMove(isHoldingLockedFilesHandler))
+ , m_hysteresis([this](PAL::HysteresisState state) { setIsHoldingLockedFiles(state == PAL::HysteresisState::Started); })
{
+ ASSERT(RunLoop::isMain());
SQLiteDatabaseTracker::setClient(this);
}
-WebSQLiteDatabaseTracker::WebSQLiteDatabaseTracker(NetworkProcess& process)
- : m_process(process)
- , m_processType(AuxiliaryProcess::ProcessType::Network)
- , m_hysteresis([this](PAL::HysteresisState state) { hysteresisUpdated(state); })
-{
- SQLiteDatabaseTracker::setClient(this);
-}
-
WebSQLiteDatabaseTracker::~WebSQLiteDatabaseTracker()
{
ASSERT(RunLoop::isMain());
@@ -58,12 +49,18 @@
SQLiteDatabaseTracker::setClient(nullptr);
if (m_hysteresis.state() == PAL::HysteresisState::Started)
- hysteresisUpdated(PAL::HysteresisState::Stopped);
+ setIsHoldingLockedFiles(false);
}
+void WebSQLiteDatabaseTracker::setIsSuspended(bool isSuspended)
+{
+ ASSERT(RunLoop::isMain());
+ m_isSuspended = isSuspended;
+}
+
void WebSQLiteDatabaseTracker::willBeginFirstTransaction()
{
- callOnMainThread([this] {
+ RunLoop::main().dispatch([this] {
m_hysteresis.start();
});
}
@@ -70,23 +67,19 @@
void WebSQLiteDatabaseTracker::didFinishLastTransaction()
{
- callOnMainThread([this] {
+ RunLoop::main().dispatch([this] {
m_hysteresis.stop();
});
}
-void WebSQLiteDatabaseTracker::hysteresisUpdated(PAL::HysteresisState state)
+void WebSQLiteDatabaseTracker::setIsHoldingLockedFiles(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
Modified: trunk/Source/WebKit/Shared/WebSQLiteDatabaseTracker.h (245068 => 245069)
--- trunk/Source/WebKit/Shared/WebSQLiteDatabaseTracker.h 2019-05-08 21:14:39 UTC (rev 245068)
+++ trunk/Source/WebKit/Shared/WebSQLiteDatabaseTracker.h 2019-05-08 21:18:48 UTC (rev 245069)
@@ -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;
+ void setIsSuspended(bool);
private:
- void hysteresisUpdated(PAL::HysteresisState);
+ void setIsHoldingLockedFiles(bool);
- AuxiliaryProcess& m_process;
- AuxiliaryProcess::ProcessType m_processType;
+ // WebCore::SQLiteDatabaseTrackerClient.
+ void willBeginFirstTransaction() final;
+ void didFinishLastTransaction() final;
+
+ IsHoldingLockedFilesHandler m_isHoldingLockedFilesHandler;
PAL::HysteresisActivity m_hysteresis;
+ bool m_isSuspended { false };
};
} // namespace WebKit
Modified: trunk/Source/WebKit/WebProcess/WebProcess.cpp (245068 => 245069)
--- trunk/Source/WebKit/WebProcess/WebProcess.cpp 2019-05-08 21:14:39 UTC (rev 245068)
+++ trunk/Source/WebKit/WebProcess/WebProcess.cpp 2019-05-08 21:18:48 UTC (rev 245069)
@@ -188,7 +188,7 @@
#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 @@
#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 @@
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 @@
unfreezeAllLayerTrees();
#if PLATFORM(IOS_FAMILY)
- m_webSQLiteDatabaseTracker = std::make_unique<WebSQLiteDatabaseTracker>(*this);
+ m_webSQLiteDatabaseTracker.setIsSuspended(false);
SQLiteDatabase::setIsDatabaseOpeningForbidden(false);
accessibilityProcessSuspendedNotification(false);
#endif
Modified: trunk/Source/WebKit/WebProcess/WebProcess.h (245068 => 245069)
--- trunk/Source/WebKit/WebProcess/WebProcess.h 2019-05-08 21:14:39 UTC (rev 245068)
+++ trunk/Source/WebKit/WebProcess/WebProcess.h 2019-05-08 21:18:48 UTC (rev 245069)
@@ -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 @@
struct WebPageCreationParameters;
struct WebPageGroupData;
struct WebPreferencesStore;
-class WebSQLiteDatabaseTracker;
struct WebsiteData;
struct WebsiteDataStoreParameters;
@@ -512,7 +512,7 @@
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)
