Changeset 251362 in webkit


Ignore:
Timestamp:
Oct 21, 2019 2:21:14 AM (5 years ago)
Author:
Carlos Garcia Campos
Message:

[GTK][WPE] IconDatabase is not thread safe yet
​https://bugs.webkit.org/show_bug.cgi?id=202980

Reviewed by Adrian Perez de Castro.

Current implementation is safer, but we still need to protect members used by both threads.

  • UIProcess/API/glib/IconDatabase.cpp:

(WebKit::IconDatabase::populatePageURLToIconURLMap):
(WebKit::IconDatabase::clearLoadedIconsTimerFired):
(WebKit::IconDatabase::checkIconURLAndSetPageURLIfNeeded):
(WebKit::IconDatabase::loadIconForPageURL):
(WebKit::IconDatabase::iconURLForPageURL):
(WebKit::IconDatabase::setIconForPageURL):
(WebKit::IconDatabase::clear):

  • UIProcess/API/glib/IconDatabase.h:
Location:
trunk/Source/WebKit
Files:
3 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WebKit/ChangeLog

    r251361 r251362  
     12019-10-21  Carlos Garcia Campos  <cgarcia@igalia.com>
     2
     3        [GTK][WPE] IconDatabase is not thread safe yet
     4        https://bugs.webkit.org/show_bug.cgi?id=202980
     5
     6        Reviewed by Adrian Perez de Castro.
     7
     8        Current implementation is safer, but we still need to protect members used by both threads.
     9
     10        * UIProcess/API/glib/IconDatabase.cpp:
     11        (WebKit::IconDatabase::populatePageURLToIconURLMap):
     12        (WebKit::IconDatabase::clearLoadedIconsTimerFired):
     13        (WebKit::IconDatabase::checkIconURLAndSetPageURLIfNeeded):
     14        (WebKit::IconDatabase::loadIconForPageURL):
     15        (WebKit::IconDatabase::iconURLForPageURL):
     16        (WebKit::IconDatabase::setIconForPageURL):
     17        (WebKit::IconDatabase::clear):
     18        * UIProcess/API/glib/IconDatabase.h:
     19
    1202019-10-21  Tim Horton  <timothy_horton@apple.com>
    221

  • trunk/Source/WebKit/UIProcess/API/glib/IconDatabase.cpp

    r250518 r251362  
    5252    , m_clearLoadedIconsTimer(RunLoop::main(), this, &IconDatabase::clearLoadedIconsTimerFired)
    5353{
     54    ASSERT(isMainThread());
    5455    m_clearLoadedIconsTimer.setPriority(RunLoopSourcePriority::ReleaseUnusedResourcesTimer);
    5556
    … …  
    108109IconDatabase::~IconDatabase()
    109110{
     111    ASSERT(isMainThread());
     112
    110113    BinarySemaphore semaphore;
    111114    m_workQueue->dispatch([&] {
    … …  
    122125bool IconDatabase::createTablesIfNeeded()
    123126{
     127    ASSERT(!isMainThread());
     128
    124129    if (m_db.tableExists("IconInfo") && m_db.tableExists("IconData") && m_db.tableExists("PageURL") && m_db.tableExists("IconDatabaseInfo"))
    125130        return false;
    … …  
    178183void IconDatabase::populatePageURLToIconURLMap()
    179184{
     185    ASSERT(!isMainThread());
     186
    180187    if (!m_db.isOpen())
    181188        return;
    … …  
    189196
    190197    auto result = query.step();
     198    LockHolder lockHolder(m_pageURLToIconURLMapLock);
    191199    while (result == SQLITE_ROW) {
    192200        m_pageURLToIconURLMap.set(query.getColumnText(0), query.getColumnText(1));
    … …  
    199207void IconDatabase::clearStatements()
    200208{
    201     RELEASE_ASSERT(m_db.isOpen());
     209    ASSERT(!isMainThread());
     210    ASSERT(m_db.isOpen());
    202211
    203212    m_iconIDForIconURLStatement = nullptr;
    … …  
    215224void IconDatabase::pruneTimerFired()
    216225{
    217     RELEASE_ASSERT(m_db.isOpen());
     226    ASSERT(!isMainThread());
     227    ASSERT(m_db.isOpen());
    218228
    219229    if (!m_pruneIconsStatement) {
    … …  
    244254void IconDatabase::startPruneTimer()
    245255{
     256    ASSERT(!isMainThread());
     257
    246258    if (!m_pruneTimer || !m_db.isOpen())
    247259        return;
    … …  
    254266void IconDatabase::clearLoadedIconsTimerFired()
    255267{
     268    ASSERT(isMainThread());
     269
     270    LockHolder lockHolder(m_loadedIconsLock);
    256271    auto now = MonotonicTime::now();
    257272    Vector<String> iconsToRemove;
    … …  
    270285void IconDatabase::startClearLoadedIconsTimer()
    271286{
     287    ASSERT(isMainThread());
     288
    272289    if (m_clearLoadedIconsTimer.isActive())
    273290        return;
    … …  
    278295Optional<int64_t> IconDatabase::iconIDForIconURL(const String& iconURL, bool& expired)
    279296{
    280     RELEASE_ASSERT(m_db.isOpen());
     297    ASSERT(!isMainThread());
     298    ASSERT(m_db.isOpen());
    281299
    282300    if (!m_iconIDForIconURLStatement) {
    … …  
    306324bool IconDatabase::setIconIDForPageURL(int64_t iconID, const String& pageURL)
    307325{
    308     RELEASE_ASSERT(m_db.isOpen());
    309     RELEASE_ASSERT(m_allowDatabaseWrite == AllowDatabaseWrite::Yes);
     326    ASSERT(!isMainThread());
     327    ASSERT(m_db.isOpen());
     328    ASSERT(m_allowDatabaseWrite == AllowDatabaseWrite::Yes);
    310329
    311330    if (!m_setIconIDForPageURLStatement) {
    … …  
    333352Vector<char> IconDatabase::iconData(int64_t iconID)
    334353{
    335     RELEASE_ASSERT(m_db.isOpen());
     354    ASSERT(!isMainThread());
     355    ASSERT(m_db.isOpen());
    336356
    337357    if (!m_iconDataStatement) {
    … …  
    359379Optional<int64_t> IconDatabase::addIcon(const String& iconURL, const Vector<char>& iconData)
    360380{
    361     RELEASE_ASSERT(m_db.isOpen());
    362     RELEASE_ASSERT(m_allowDatabaseWrite == AllowDatabaseWrite::Yes);
     381    ASSERT(!isMainThread());
     382    ASSERT(m_db.isOpen());
     383    ASSERT(m_allowDatabaseWrite == AllowDatabaseWrite::Yes);
    363384
    364385    if (!m_addIconStatement) {
    … …  
    401422void IconDatabase::updateIconTimestamp(int64_t iconID, int64_t timestamp)
    402423{
    403     RELEASE_ASSERT(m_db.isOpen());
    404     RELEASE_ASSERT(m_allowDatabaseWrite == AllowDatabaseWrite::Yes);
     424    ASSERT(!isMainThread());
     425    ASSERT(m_db.isOpen());
     426    ASSERT(m_allowDatabaseWrite == AllowDatabaseWrite::Yes);
    405427
    406428    if (!m_updateIconTimestampStatement) {
    … …  
    424446void IconDatabase::deleteIcon(int64_t iconID)
    425447{
    426     RELEASE_ASSERT(m_db.isOpen());
    427     RELEASE_ASSERT(m_allowDatabaseWrite == AllowDatabaseWrite::Yes);
     448    ASSERT(!isMainThread());
     449    ASSERT(m_db.isOpen());
     450    ASSERT(m_allowDatabaseWrite == AllowDatabaseWrite::Yes);
    428451
    429452    if (!m_deletePageURLsForIconStatement) {
    … …  
    470493void IconDatabase::checkIconURLAndSetPageURLIfNeeded(const String& iconURL, const String& pageURL, AllowDatabaseWrite allowDatabaseWrite, CompletionHandler<void(bool, bool)>&& completionHandler)
    471494{
     495    ASSERT(isMainThread());
     496
    472497    m_workQueue->dispatch([this, protectedThis = makeRef(*this), iconURL = iconURL.isolatedCopy(), pageURL = pageURL.isolatedCopy(), allowDatabaseWrite, completionHandler = WTFMove(completionHandler)]() mutable {
    473498        bool result = false;
    … …  
    476501            bool canWriteToDatabase = m_allowDatabaseWrite == AllowDatabaseWrite::Yes && allowDatabaseWrite == AllowDatabaseWrite::Yes;
    477502            bool expired = false;
    478             if (m_pageURLToIconURLMap.get(pageURL) == iconURL)
     503            String cachedIconURL;
     504            {
     505                LockHolder lockHolder(m_pageURLToIconURLMapLock);
     506                cachedIconURL = m_pageURLToIconURLMap.get(pageURL);
     507            }
     508            if (cachedIconURL == iconURL)
    479509                result = true;
    480510            else if (auto iconID = iconIDForIconURL(iconURL, expired)) {
    … …  
    487517                    result = true;
    488518                    if (!canWriteToDatabase || setIconIDForPageURL(iconID.value(), pageURL)) {
     519                        LockHolder lockHolder(m_pageURLToIconURLMapLock);
    489520                        m_pageURLToIconURLMap.set(pageURL, iconURL);
    490521                        changed = true;
    491522                    }
    492523                }
    493             } else if (!canWriteToDatabase && m_loadedIcons.contains(iconURL)) {
    494                 // Found in memory cache.
    495                 result = true;
    496                 m_pageURLToIconURLMap.set(pageURL, iconURL);
    497                 changed = true;
     524            } else if (!canWriteToDatabase) {
     525                bool foundInMemoryCache;
     526                {
     527                    LockHolder lockHolder(m_loadedIconsLock);
     528                    foundInMemoryCache = m_loadedIcons.contains(iconURL);
     529                }
     530
     531                if (foundInMemoryCache) {
     532                    result = true;
     533                    LockHolder lockHolder(m_pageURLToIconURLMapLock);
     534                    m_pageURLToIconURLMap.set(pageURL, iconURL);
     535                    changed = true;
     536                }
    498537            }
    499538        }
    … …  
    507546void IconDatabase::loadIconForPageURL(const String& pageURL, AllowDatabaseWrite allowDatabaseWrite, CompletionHandler<void(NativeImagePtr&&)>&& completionHandler)
    508547{
     548    ASSERT(isMainThread());
     549
    509550    m_workQueue->dispatch([this, protectedThis = makeRef(*this), pageURL = pageURL.isolatedCopy(), allowDatabaseWrite, timestamp = WallTime::now().secondsSinceEpoch(), completionHandler = WTFMove(completionHandler)]() mutable {
    510551        Optional<int64_t> iconID;
    511552        Vector<char> iconData;
    512         auto iconURL = m_pageURLToIconURLMap.get(pageURL);
     553        String iconURL;
     554        {
     555            LockHolder lockHolder(m_pageURLToIconURLMapLock);
     556            iconURL = m_pageURLToIconURLMap.get(pageURL);
     557        }
    513558        if (m_db.isOpen() && !iconURL.isEmpty()) {
    514559            bool expired;
    515560            iconID = iconIDForIconURL(iconURL, expired);
    516             if (iconID && !m_loadedIcons.contains(iconURL)) {
    517                 iconData = this->iconData(iconID.value());
    518                 m_loadedIcons.set(iconURL, std::make_pair<NativeImagePtr, MonotonicTime>(nullptr, { }));
     561            if (iconID) {
     562                LockHolder lockHolder(m_loadedIconsLock);
     563                if (!m_loadedIcons.contains(iconURL)) {
     564                    iconData = this->iconData(iconID.value());
     565                    m_loadedIcons.set(iconURL, std::make_pair<NativeImagePtr, MonotonicTime>(nullptr, { }));
     566                }
    519567            }
    520568            bool canWriteToDatabase = m_allowDatabaseWrite == AllowDatabaseWrite::Yes && allowDatabaseWrite == AllowDatabaseWrite::Yes;
    … …  
    529577            }
    530578
     579            LockHolder lockHolder(m_loadedIconsLock);
    531580            auto it = m_loadedIcons.find(iconURL);
    532581            if (it != m_loadedIcons.end() && it->value.first) {
    … …  
    534583                it->value.second = MonotonicTime::now();
    535584                startClearLoadedIconsTimer();
     585                lockHolder.unlockEarly();
    536586                completionHandler(WTFMove(icon));
    537587                return;
    … …  
    550600            auto icon = addResult.iterator->value.first;
    551601            startClearLoadedIconsTimer();
     602            lockHolder.unlockEarly();
    552603            completionHandler(WTFMove(icon));
    553604        });
    … …  
    557608String IconDatabase::iconURLForPageURL(const String& pageURL)
    558609{
     610    ASSERT(isMainThread());
     611
     612    LockHolder lockHolder(m_pageURLToIconURLMapLock);
    559613    return m_pageURLToIconURLMap.get(pageURL);
    560614}
    … …  
    562616void IconDatabase::setIconForPageURL(const String& iconURL, const unsigned char* iconData, size_t iconDataSize, const String& pageURL, AllowDatabaseWrite allowDatabaseWrite, CompletionHandler<void(bool)>&& completionHandler)
    563617{
     618    ASSERT(isMainThread());
     619
    564620    // If database write is not allowed load the icon to cache it in memory only.
    565621    if (m_allowDatabaseWrite == AllowDatabaseWrite::No || allowDatabaseWrite == AllowDatabaseWrite::No) {
    566622        bool result = true;
    567         auto addResult = m_loadedIcons.set(iconURL, std::make_pair<NativeImagePtr, MonotonicTime>(nullptr, { }));
    568         if (iconDataSize) {
    569             auto image = BitmapImage::create();
    570             if (image->setData(SharedBuffer::create(iconData, iconDataSize), true) < EncodedDataStatus::SizeAvailable)
    571                 result = false;
    572             else
    573                 addResult.iterator->value.first = image->nativeImageForCurrentFrame();
     623        {
     624            LockHolder lockHolder(m_loadedIconsLock);
     625            auto addResult = m_loadedIcons.set(iconURL, std::make_pair<NativeImagePtr, MonotonicTime>(nullptr, { }));
     626            if (iconDataSize) {
     627                auto image = BitmapImage::create();
     628                if (image->setData(SharedBuffer::create(iconData, iconDataSize), true) < EncodedDataStatus::SizeAvailable)
     629                    result = false;
     630                else
     631                    addResult.iterator->value.first = image->nativeImageForCurrentFrame();
     632            }
    574633        }
    575634        startClearLoadedIconsTimer();
    576635        m_workQueue->dispatch([this, protectedThis = makeRef(*this), result, iconURL = iconURL.isolatedCopy(), pageURL = pageURL.isolatedCopy(), completionHandler = WTFMove(completionHandler)]() mutable {
    577             m_pageURLToIconURLMap.set(pageURL, iconURL);
     636            {
     637                LockHolder lockHolder(m_pageURLToIconURLMapLock);
     638                m_pageURLToIconURLMap.set(pageURL, iconURL);
     639            }
    578640            RunLoop::main().dispatch([result, completionHandler = WTFMove(completionHandler)]() mutable {
    579641                completionHandler(result);
    … …  
    599661            if (iconID) {
    600662                result = true;
    601                 if (setIconIDForPageURL(iconID.value(), pageURL))
     663                if (setIconIDForPageURL(iconID.value(), pageURL)) {
     664                    LockHolder lockHolder(m_pageURLToIconURLMapLock);
    602665                    m_pageURLToIconURLMap.set(pageURL, iconURL);
     666                }
    603667            }
    604668
    … …  
    614678void IconDatabase::clear(CompletionHandler<void()>&& completionHandler)
    615679{
    616     m_loadedIcons.clear();
     680    ASSERT(isMainThread());
     681
     682    {
     683        LockHolder lockHolder(m_loadedIconsLock);
     684        m_loadedIcons.clear();
     685    }
    617686    m_workQueue->dispatch([this, protectedThis = makeRef(*this), completionHandler = WTFMove(completionHandler)]() mutable {
    618         m_pageURLToIconURLMap.clear();
     687        {
     688            LockHolder lockHolder(m_pageURLToIconURLMapLock);
     689            m_pageURLToIconURLMap.clear();
     690        }
    619691
    620692        if (m_db.isOpen() && m_allowDatabaseWrite == AllowDatabaseWrite::Yes) {
    621             m_pageURLToIconURLMap.clear();
    622 
    623693            clearStatements();
    624694            m_db.clearAllTables();

  • trunk/Source/WebKit/UIProcess/API/glib/IconDatabase.h

    r250518 r251362  
    2525#include <wtf/CompletionHandler.h>
    2626#include <wtf/HashMap.h>
     27#include <wtf/Lock.h>
    2728#include <wtf/ThreadSafeRefCounted.h>
    2829#include <wtf/WorkQueue.h>
    … …  
    7071    WebCore::SQLiteDatabase m_db;
    7172    HashMap<String, String> m_pageURLToIconURLMap;
     73    Lock m_pageURLToIconURLMapLock;
    7274    HashMap<String, std::pair<WebCore::NativeImagePtr, MonotonicTime>> m_loadedIcons;
     75    Lock m_loadedIconsLock;
    7376
    7477    std::unique_ptr<WebCore::SQLiteStatement> m_iconIDForIconURLStatement;
Note: See TracChangeset for help on using the changeset viewer.