Changeset 245069 in webkit


Ignore:
Timestamp:
May 8, 2019 2:18:48 PM (5 years ago)
Author:
Chris Dumez
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:
Location:
trunk/Source/WebKit
Files:
6 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WebKit/ChangeLog

    r245067 r245069  
     12019-05-08  Chris Dumez  <cdumez@apple.com>
     2
     3        Regression: Crash at WebKit: PAL::HysteresisActivity::start
     4        https://bugs.webkit.org/show_bug.cgi?id=197666
     5        <rdar://problem/50037153>
     6
     7        Reviewed by Geoffrey Garen.
     8
     9        We've recently made it so that the WebContent process destroys its WebSQLiteDatabaseTracker when preparing
     10        for process suspension and then re-constructs it when resuming. The issue is that the WebSQLiteDatabaseTracker
     11        internal implementation was calling callOnMainThread() and capturing |this| to start/stop its HysteresisActivity.
     12        As a result, |this| could be dead by the time we're on the main thread and we'd crash.
     13
     14        To address the issue, we no longer destroy the WebSQLiteDatabaseTracker when preparing to suspend. Instead, we
     15        set a 'isSuspended' flag on the WebSQLiteDatabaseTracker so that it stops notifying the WebProcess of changes.
     16
     17        Also clean up the class a bit so that:
     18        1. The constructor takes in a WTF::Function instead of a NetworkProcess / WebProcess reference. This is provides
     19           better layering. The WebSQLiteDatabaseTracker should not need to know anything about those objects.
     20        2. Use RunLoop::main().dispatch() instead of callOnMainThread() since we're in WebKit2 code.
     21
     22        * NetworkProcess/NetworkProcess.cpp:
     23        (WebKit::NetworkProcess::NetworkProcess):
     24        * Shared/WebSQLiteDatabaseTracker.cpp:
     25        (WebKit::WebSQLiteDatabaseTracker::WebSQLiteDatabaseTracker):
     26        (WebKit::WebSQLiteDatabaseTracker::~WebSQLiteDatabaseTracker):
     27        (WebKit::WebSQLiteDatabaseTracker::willBeginFirstTransaction):
     28        (WebKit::WebSQLiteDatabaseTracker::didFinishLastTransaction):
     29        (WebKit::WebSQLiteDatabaseTracker::hysteresisUpdated): Deleted.
     30        * Shared/WebSQLiteDatabaseTracker.h:
     31        * WebProcess/WebProcess.cpp:
     32        (WebKit::m_nonVisibleProcessCleanupTimer):
     33        (WebKit::WebProcess::initializeSQLiteDatabaseTracker):
     34        (WebKit::WebProcess::cancelPrepareToSuspend):
     35        (WebKit::WebProcess::processDidResume):
     36        (WebKit::m_webSQLiteDatabaseTracker): Deleted.
     37        * WebProcess/WebProcess.h:
     38
    1392019-05-08  Tim Horton  <timothy_horton@apple.com>
    240

  • trunk/Source/WebKit/NetworkProcess/NetworkProcess.cpp

    r245053 r245069  
    134134#endif
    135135#if PLATFORM(IOS_FAMILY)
    136     , m_webSQLiteDatabaseTracker(*this)
     136    , m_webSQLiteDatabaseTracker([this](bool isHoldingLockedFiles) { parentProcessConnection()->send(Messages::NetworkProcessProxy::SetIsHoldingLockedFiles(isHoldingLockedFiles), 0); })
    137137#endif
    138138{

  • trunk/Source/WebKit/Shared/WebSQLiteDatabaseTracker.cpp

    r243939 r245069  
    3232#include "WebProcessProxyMessages.h"
    3333#include <WebCore/SQLiteDatabaseTracker.h>
    34 #include <wtf/MainThread.h>
    3534
    3635namespace WebKit {
    3736using namespace WebCore;
    3837
    39 WebSQLiteDatabaseTracker::WebSQLiteDatabaseTracker(WebProcess& process)
    40     : m_process(process)
    41     , m_processType(AuxiliaryProcess::ProcessType::WebContent)
    42     , m_hysteresis([this](PAL::HysteresisState state) { hysteresisUpdated(state); })
     38WebSQLiteDatabaseTracker::WebSQLiteDatabaseTracker(IsHoldingLockedFilesHandler&& isHoldingLockedFilesHandler)
     39    : m_isHoldingLockedFilesHandler(WTFMove(isHoldingLockedFilesHandler))
     40    , m_hysteresis([this](PAL::HysteresisState state) { setIsHoldingLockedFiles(state == PAL::HysteresisState::Started); })
    4341{
    44     SQLiteDatabaseTracker::setClient(this);
    45 }
    46 
    47 WebSQLiteDatabaseTracker::WebSQLiteDatabaseTracker(NetworkProcess& process)
    48     : m_process(process)
    49     , m_processType(AuxiliaryProcess::ProcessType::Network)
    50     , m_hysteresis([this](PAL::HysteresisState state) { hysteresisUpdated(state); })
    51 {
     42    ASSERT(RunLoop::isMain());
    5243    SQLiteDatabaseTracker::setClient(this);
    5344}
    … …  
    5950
    6051    if (m_hysteresis.state() == PAL::HysteresisState::Started)
    61         hysteresisUpdated(PAL::HysteresisState::Stopped);
     52        setIsHoldingLockedFiles(false);
     53}
     54
     55void WebSQLiteDatabaseTracker::setIsSuspended(bool isSuspended)
     56{
     57    ASSERT(RunLoop::isMain());
     58    m_isSuspended = isSuspended;
    6259}
    6360
    6461void WebSQLiteDatabaseTracker::willBeginFirstTransaction()
    6562{
    66     callOnMainThread([this] {
     63    RunLoop::main().dispatch([this] {
    6764        m_hysteresis.start();
    6865    });
    … …  
    7168void WebSQLiteDatabaseTracker::didFinishLastTransaction()
    7269{
    73     callOnMainThread([this] {
     70    RunLoop::main().dispatch([this] {
    7471        m_hysteresis.stop();
    7572    });
    7673}
    7774
    78 void WebSQLiteDatabaseTracker::hysteresisUpdated(PAL::HysteresisState state)
     75void WebSQLiteDatabaseTracker::setIsHoldingLockedFiles(bool isHoldingLockedFiles)
    7976{
    80     switch (m_processType) {
    81     case AuxiliaryProcess::ProcessType::WebContent:
    82         m_process.parentProcessConnection()->send(Messages::WebProcessProxy::SetIsHoldingLockedFiles(state == PAL::HysteresisState::Started), 0);
    83         break;
    84     case AuxiliaryProcess::ProcessType::Network:
    85         m_process.parentProcessConnection()->send(Messages::NetworkProcessProxy::SetIsHoldingLockedFiles(state == PAL::HysteresisState::Started), 0);
    86         break;
    87     default:
    88         ASSERT_NOT_REACHED();
    89     }
     77    ASSERT(RunLoop::isMain());
     78
     79    if (m_isSuspended)
     80        return;
     81
     82    m_isHoldingLockedFilesHandler(isHoldingLockedFiles);
    9083}
    9184

  • trunk/Source/WebKit/Shared/WebSQLiteDatabaseTracker.h

    r243939 r245069  
    3232namespace WebKit {
    3333
    34 class NetworkProcess;
    35 class WebProcess;
    36 
    37 class WebSQLiteDatabaseTracker : public WebCore::SQLiteDatabaseTrackerClient {
     34class WebSQLiteDatabaseTracker final : public WebCore::SQLiteDatabaseTrackerClient {
    3835    WTF_MAKE_NONCOPYABLE(WebSQLiteDatabaseTracker)
    3936public:
    40     explicit WebSQLiteDatabaseTracker(WebProcess&);
    41     explicit WebSQLiteDatabaseTracker(NetworkProcess&);
     37    using IsHoldingLockedFilesHandler = Function<void(bool)>;
     38    explicit WebSQLiteDatabaseTracker(IsHoldingLockedFilesHandler&&);
    4239
    4340    ~WebSQLiteDatabaseTracker();
    4441
    45     // WebCore::SQLiteDatabaseTrackerClient
    46     void willBeginFirstTransaction() override;
    47     void didFinishLastTransaction() override;
     42    void setIsSuspended(bool);
    4843
    4944private:
    50     void hysteresisUpdated(PAL::HysteresisState);
     45    void setIsHoldingLockedFiles(bool);
    5146
    52     AuxiliaryProcess& m_process;
    53     AuxiliaryProcess::ProcessType m_processType;
     47    // WebCore::SQLiteDatabaseTrackerClient.
     48    void willBeginFirstTransaction() final;
     49    void didFinishLastTransaction() final;
     50
     51    IsHoldingLockedFilesHandler m_isHoldingLockedFilesHandler;
    5452    PAL::HysteresisActivity m_hysteresis;
     53    bool m_isSuspended { false };
    5554};
    5655

  • trunk/Source/WebKit/WebProcess/WebProcess.cpp

    r244979 r245069  
    189189    , m_nonVisibleProcessCleanupTimer(*this, &WebProcess::nonVisibleProcessCleanupTimerFired)
    190190#if PLATFORM(IOS_FAMILY)
    191     , m_webSQLiteDatabaseTracker(std::make_unique<WebSQLiteDatabaseTracker>(*this))
     191    , m_webSQLiteDatabaseTracker([this](bool isHoldingLockedFiles) { parentProcessConnection()->send(Messages::WebProcessProxy::SetIsHoldingLockedFiles(isHoldingLockedFiles), 0); })
    192192#endif
    193193{
    … …  
    14691469
    14701470#if PLATFORM(IOS_FAMILY)
    1471     m_webSQLiteDatabaseTracker = nullptr;
     1471    m_webSQLiteDatabaseTracker.setIsSuspended(true);
    14721472    SQLiteDatabase::setIsDatabaseOpeningForbidden(true);
    14731473    if (DatabaseTracker::isInitialized())
    … …  
    15201520
    15211521#if PLATFORM(IOS_FAMILY)
    1522     m_webSQLiteDatabaseTracker = std::make_unique<WebSQLiteDatabaseTracker>(*this);
     1522    m_webSQLiteDatabaseTracker.setIsSuspended(false);
    15231523    SQLiteDatabase::setIsDatabaseOpeningForbidden(false);
    15241524    accessibilityProcessSuspendedNotification(false);
    … …  
    15961596   
    15971597#if PLATFORM(IOS_FAMILY)
    1598     m_webSQLiteDatabaseTracker = std::make_unique<WebSQLiteDatabaseTracker>(*this);
     1598    m_webSQLiteDatabaseTracker.setIsSuspended(false);
    15991599    SQLiteDatabase::setIsDatabaseOpeningForbidden(false);
    16001600    accessibilityProcessSuspendedNotification(false);

  • trunk/Source/WebKit/WebProcess/WebProcess.h

    r244091 r245069  
    3838#include "WebInspectorInterruptDispatcher.h"
    3939#include "WebProcessCreationParameters.h"
     40#include "WebSQLiteDatabaseTracker.h"
    4041#include <WebCore/ActivityState.h>
    4142#include <WebCore/RegistrableDomain.h>
    … …  
    113114struct WebPageGroupData;
    114115struct WebPreferencesStore;
    115 class WebSQLiteDatabaseTracker;
    116116struct WebsiteData;
    117117struct WebsiteDataStoreParameters;
    … …  
    513513
    514514#if PLATFORM(IOS_FAMILY)
    515     std::unique_ptr<WebSQLiteDatabaseTracker> m_webSQLiteDatabaseTracker;
     515    WebSQLiteDatabaseTracker m_webSQLiteDatabaseTracker;
    516516    ProcessTaskStateObserver m_taskStateObserver { *this };
    517517#endif
Note: See TracChangeset for help on using the changeset viewer.