Changeset 117744 in webkit


Ignore:
Timestamp:
May 21, 2012 1:14:46 AM (12 years ago)
Author:
kling@webkit.org
Message:

REGRESSION(r117501): IconDatabase asserts on startup in synchronousIconForPageURL().
<http://webkit.org/b/86935>
<rdar://problem/11480012>

Reviewed by Anders Carlsson.

Source/WebCore:

  • Correctly set m_retainOrReleaseIconRequested to true in retainIconForPageURL(). This was causing the assertions, as we would end up doing nothing until the first icon release request came in.
  • Require that m_urlsToRetainOrReleaseLock be held when accessing m_retainOrReleaseIconRequested. This removes a possible race condition in double checked locking.
  • Swap over the retain/release work queues while holding m_urlsToRetainOrReleaseLock and release it right away to avoid sitting on the lock while updating the database.
  • loader/icon/IconDatabase.cpp:

(WebCore::IconDatabase::synchronousIconForPageURL):
(WebCore::IconDatabase::retainIconForPageURL):
(WebCore::IconDatabase::releaseIconForPageURL):
(WebCore::IconDatabase::retainedPageURLCount):
(WebCore::IconDatabase::performURLImport):
(WebCore::IconDatabase::syncThreadMainLoop):
(WebCore::IconDatabase::performPendingRetainAndReleaseOperations):

  • loader/icon/IconDatabase.h:

(IconDatabase):

Source/WTF:

Added a swap() to HashCountedSet.

  • wtf/HashCountedSet.h:

(HashCountedSet::swap):

Location:
trunk/Source
Files:
5 edited

Legend:

Unmodified
Added
Removed
  • trunk/Source/WTF/ChangeLog

    r117478 r117744  
     12012-05-21  Andreas Kling  <kling@webkit.org>
     2
     3        REGRESSION(r117501): IconDatabase asserts on startup in synchronousIconForPageURL().
     4        <http://webkit.org/b/86935>
     5        <rdar://problem/11480012>
     6
     7        Reviewed by Anders Carlsson.
     8
     9        Added a swap() to HashCountedSet.
     10
     11        * wtf/HashCountedSet.h:
     12        (HashCountedSet::swap):
     13
    1142012-05-16  Geoffrey Garen  <ggaren@apple.com>
    215
  • trunk/Source/WTF/wtf/HashCountedSet.h

    r112555 r117744  
    4040       
    4141        HashCountedSet() {}
     42
     43        void swap(HashCountedSet&);
    4244       
    4345        int size() const;
     
    7678        ImplType m_impl;
    7779    };
     80
     81    template<typename Value, typename HashFunctions, typename Traits>
     82    inline void HashCountedSet<Value, HashFunctions, Traits>::swap(HashCountedSet& other)
     83    {
     84        m_impl.swap(other.m_impl);
     85    }
    7886   
    7987    template<typename Value, typename HashFunctions, typename Traits>
  • trunk/Source/WebCore/ChangeLog

    r117743 r117744  
     12012-05-18  Andreas Kling  <kling@webkit.org>
     2
     3        REGRESSION(r117501): IconDatabase asserts on startup in synchronousIconForPageURL().
     4        <http://webkit.org/b/86935>
     5        <rdar://problem/11480012>
     6
     7        Reviewed by Anders Carlsson.
     8
     9        - Correctly set m_retainOrReleaseIconRequested to true in retainIconForPageURL().
     10          This was causing the assertions, as we would end up doing nothing until the first
     11          icon release request came in.
     12
     13        - Require that m_urlsToRetainOrReleaseLock be held when accessing m_retainOrReleaseIconRequested.
     14          This removes a possible race condition in double checked locking.
     15
     16        - Swap over the retain/release work queues while holding m_urlsToRetainOrReleaseLock
     17          and release it right away to avoid sitting on the lock while updating the database.
     18
     19        * loader/icon/IconDatabase.cpp:
     20        (WebCore::IconDatabase::synchronousIconForPageURL):
     21        (WebCore::IconDatabase::retainIconForPageURL):
     22        (WebCore::IconDatabase::releaseIconForPageURL):
     23        (WebCore::IconDatabase::retainedPageURLCount):
     24        (WebCore::IconDatabase::performURLImport):
     25        (WebCore::IconDatabase::syncThreadMainLoop):
     26        (WebCore::IconDatabase::performPendingRetainAndReleaseOperations):
     27        * loader/icon/IconDatabase.h:
     28        (IconDatabase):
     29
    1302012-05-21  Kent Tamura  <tkent@chromium.org>
    231
  • trunk/Source/WebCore/loader/icon/IconDatabase.cpp

    r117625 r117744  
    224224    MutexLocker locker(m_urlAndIconLock);
    225225
    226     if (m_retainOrReleaseIconRequested)
    227         performPendingRetainAndReleaseOperations();
     226    performPendingRetainAndReleaseOperations();
    228227   
    229228    String pageURLCopy; // Creates a null string for easy testing
     
    399398    if (!isEnabled() || !documentCanHaveIcon(pageURL))
    400399        return;
    401        
    402     MutexLocker locker(m_urlsToRetainOrReleaseLock);
    403     m_urlsToRetain.add(pageURL);
     400
     401    {
     402        MutexLocker locker(m_urlsToRetainOrReleaseLock);
     403        m_urlsToRetain.add(pageURL);
     404        m_retainOrReleaseIconRequested = true;
     405    }
     406
    404407    scheduleOrDeferSyncTimer();
    405408}
     
    449452        return;
    450453
    451     MutexLocker locker(m_urlsToRetainOrReleaseLock);
    452     m_urlsToRelease.add(pageURL);
    453     m_retainOrReleaseIconRequested = true;
     454    {
     455        MutexLocker locker(m_urlsToRetainOrReleaseLock);
     456        m_urlsToRelease.add(pageURL);
     457        m_retainOrReleaseIconRequested = true;
     458    }
    454459    scheduleOrDeferSyncTimer();
    455460}
     
    745750{
    746751    MutexLocker locker(m_urlAndIconLock);
    747 
    748     if (m_retainOrReleaseIconRequested)
    749         performPendingRetainAndReleaseOperations();
    750 
     752    performPendingRetainAndReleaseOperations();
    751753    return m_retainedPageURLs.size();
    752754}
     
    13351337        MutexLocker locker(m_urlAndIconLock);
    13361338
    1337         if (m_retainOrReleaseIconRequested)
    1338             performPendingRetainAndReleaseOperations();
     1339        performPendingRetainAndReleaseOperations();
    13391340
    13401341        for (unsigned i = 0; i < urls.size(); ++i) {
     
    14191420            break;
    14201421
    1421         if (m_retainOrReleaseIconRequested) {
     1422        {
    14221423            MutexLocker locker(m_urlAndIconLock);
    1423             // Previous flag check was done outside of the lock and flag could be changed by another thread.
    1424             // Do not move mutex outside to avoid unnecessary locking on every loop, but recheck the flag under mutex.
    1425             if (m_retainOrReleaseIconRequested)
    1426                 performPendingRetainAndReleaseOperations();
     1424            performPendingRetainAndReleaseOperations();
    14271425        }
    14281426       
     
    15121510void IconDatabase::performPendingRetainAndReleaseOperations()
    15131511{
    1514     ASSERT(m_retainOrReleaseIconRequested);
    1515 
    15161512    // NOTE: The caller is assumed to hold m_urlAndIconLock.
    15171513    ASSERT(!m_urlAndIconLock.tryLock());
    15181514
    1519     MutexLocker vectorLocker(m_urlsToRetainOrReleaseLock);
    1520 
    1521     for (HashCountedSet<String>::const_iterator it = m_urlsToRetain.begin(), end = m_urlsToRetain.end(); it != end; ++it)
     1515    HashCountedSet<String> toRetain;
     1516    HashCountedSet<String> toRelease;
     1517
     1518    {
     1519        MutexLocker pendingWorkLocker(m_urlsToRetainOrReleaseLock);
     1520        if (!m_retainOrReleaseIconRequested)
     1521            return;
     1522
     1523        // Make a copy of the URLs to retain and/or release so we can release m_urlsToRetainOrReleaseLock ASAP.
     1524        // Holding m_urlAndIconLock protects this function from being re-entered.
     1525
     1526        toRetain.swap(m_urlsToRetain);
     1527        toRelease.swap(m_urlsToRelease);
     1528        m_retainOrReleaseIconRequested = false;
     1529    }
     1530
     1531    for (HashCountedSet<String>::const_iterator it = toRetain.begin(), end = toRetain.end(); it != end; ++it)
    15221532        performRetainIconForPageURL(it->first, it->second);
    1523     for (HashCountedSet<String>::const_iterator it = m_urlsToRelease.begin(), end = m_urlsToRelease.end(); it != end; ++it)
     1533    for (HashCountedSet<String>::const_iterator it = toRelease.begin(), end = toRelease.end(); it != end; ++it)
    15241534        performReleaseIconForPageURL(it->first, it->second);
    1525 
    1526     m_urlsToRetain.clear();
    1527     m_urlsToRelease.clear();
    1528     m_retainOrReleaseIconRequested = false;
    15291535}
    15301536
  • trunk/Source/WebCore/loader/icon/IconDatabase.h

    r117501 r117744  
    166166    bool m_syncThreadHasWorkToDo;
    167167    bool m_disabledSuddenTerminationForSyncThread;
    168     bool m_retainOrReleaseIconRequested;
    169168
    170169    Mutex m_urlAndIconLock;
     
    189188    HashCountedSet<String> m_urlsToRetain;
    190189    HashCountedSet<String> m_urlsToRelease;
     190    bool m_retainOrReleaseIconRequested;
    191191
    192192// *** Sync Thread Only ***
Note: See TracChangeset for help on using the changeset viewer.