Bug 31482 – Refactor DatabaseTracker.h and eliminate all #ifdefs

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dumitru Daniliuc]

DatabaseTracker.h has a lot of methods that are not used by anybody in WebCore/storage. In order to avoid having lots of no-op methods in the Chromium's DatabaseTracker implementation, we #ifdef'd them out when !PLATFORM(CHROMIUM). This looks ugly and should be refactored.

[Dumitru Daniliuc]

After talking to dglazkov about this, I think our plan is to:

1. Change DatabaseTracker.h to only define the functions needed by other WebCore/storage classes.
2. Add another interface (DatabaseManager? DatabaseController?) that would define quota management functions, and other "utility" functions related to databases, such as delete database, list all databases for an origin, etc.
3. Make WebCore/storage/chromium/DatabaseTrackerChromium.cpp implement the new DatabaseTracker.h interface (and ignore DatabaseManager.h).
4. Move a lot of stuff from WebCore/storage/DatabaseTracker.cpp into WebCore/storage/DatabaseManager.cpp.
5. Change classes in WebKit/{win|mac} to use DatabaseManager instead of DatabaseTracker.


Brady, Anders, Sam: any objections/suggestions?

[Michael Nordman]

There are other changes in the works to bring WebDatabases to Workers. Let's coordinate on that front too, ericu is poking at that now.

[Dumitru Daniliuc]

Created attachment 52090 [details]
patch #1: remove the dependency on OriginQuotaManager from DatabaseTracker.h

[WebKit Review Bot]

Attachment 52090 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/ChangeLog:6:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric U.]

Comment on attachment 52090 [details]
patch #1: remove the dependency on OriginQuotaManager from DatabaseTracker.h

> Index: WebCore/storage/DatabaseTracker.cpp
> ===================================================================
> --- WebCore/storage/DatabaseTracker.cpp	(revision 56810)
> +++ WebCore/storage/DatabaseTracker.cpp	(working copy)
> @@ -49,20 +49,13 @@
>  
>  using namespace std;
>  
> -namespace WebCore {
> -
> -OriginQuotaManager& DatabaseTracker::originQuotaManagerNoLock()
> +static WebCore::OriginQuotaManager& originQuotaManager()
>  {
> -    ASSERT(m_quotaManager);
> -    return *m_quotaManager;
> +    DEFINE_STATIC_LOCAL(WebCore::OriginQuotaManager, quotaManager, ());

Does this have to be a static local?  Could we just create it in the DatabaseTracker constructor now?
If it's always accessed from inside non-static DatabaseTracker methods, that should be simple.
And then we don't even need the accessor function; it can just be a data member.

> +    return quotaManager;
>  }
>  
> -OriginQuotaManager& DatabaseTracker::originQuotaManager()
> -{
> -    MutexLocker lockDatabase(m_databaseGuard);
> -    populateOrigins();
> -    return originQuotaManagerNoLock();
> -}
> +namespace WebCore {
>  
>  DatabaseTracker& DatabaseTracker::tracker()
>  {
> @@ -74,6 +67,9 @@ DatabaseTracker::DatabaseTracker()
>      : m_client(0)
>  {
>      SQLiteFileSystem::registerSQLiteVFS();
> +
> +    MutexLocker lockDatabase(m_databaseGuard);
> +    populateOrigins();
>  }
>  
>  void DatabaseTracker::setDatabaseDirectoryPath(const String& path)
> @@ -132,16 +128,14 @@ bool DatabaseTracker::canEstablishDataba
>      unsigned long long requirement;
>      unsigned long long tempUsage;
>      {
> -        Locker<OriginQuotaManager> locker(originQuotaManager());
>          MutexLocker lockDatabase(m_databaseGuard);
> +        Locker<OriginQuotaManager> quotaManagerLocker(originQuotaManager());
>  
>          if (!canCreateDatabase(origin, name))
>              return false;
>  
>          recordCreatingDatabase(origin, name);
>  
> -        populateOrigins();
> -
>          // Since we're imminently opening a database within this context's origin, make sure this origin is being tracked by the QuotaTracker
>          // by fetching its current usage now.
>          unsigned long long usage = usageForOriginNoLock(origin);
> @@ -192,7 +186,6 @@ bool DatabaseTracker::hasEntryForOriginN
>  bool DatabaseTracker::hasEntryForOrigin(SecurityOrigin* origin)
>  {
>      MutexLocker lockDatabase(m_databaseGuard);
> -    populateOrigins();
>      return hasEntryForOriginNoLock(origin);
>  }
>  
> @@ -218,11 +211,17 @@ unsigned long long DatabaseTracker::getM
>      ASSERT(currentThread() == database->scriptExecutionContext()->databaseThread()->getThreadID());
>      // The maximum size for a database is the full quota for its origin, minus the current usage within the origin,
>      // plus the current usage of the given database
> -    Locker<OriginQuotaManager> locker(originQuotaManager());
> +    Locker<OriginQuotaManager> quotaManagerLocker(originQuotaManager());
>      SecurityOrigin* origin = database->securityOrigin();
>      return quotaForOrigin(origin) - originQuotaManager().diskUsage(origin) + SQLiteFileSystem::getDatabaseFileSize(database->fileName());
>  }
>  
> +void DatabaseTracker::databaseChanged(Database* database)
> +{
> +    Locker<OriginQuotaManager> quotaManagerLocker(originQuotaManager());
> +    originQuotaManager().markDatabase(database);
> +}
> +
>  String DatabaseTracker::originPath(SecurityOrigin* origin) const
>  {
>      return SQLiteFileSystem::appendDatabaseFileNameToPath(m_databaseDirectoryPath.threadsafeCopy(), origin->databaseIdentifier());
> @@ -231,7 +230,7 @@ String DatabaseTracker::originPath(Secur
>  String DatabaseTracker::fullPathForDatabaseNoLock(SecurityOrigin* origin, const String& name, bool createIfNotExists)
>  {
>      ASSERT(!m_databaseGuard.tryLock());
> -    ASSERT(!originQuotaManagerNoLock().tryLock());
> +    ASSERT(!originQuotaManager().tryLock());
>  
>      for (HashSet<ProposedDatabase*>::iterator iter = m_proposedDatabases.begin(); iter != m_proposedDatabases.end(); ++iter)
>          if ((*iter)->first == origin && (*iter)->second.name() == name)
> @@ -275,17 +274,16 @@ String DatabaseTracker::fullPathForDatab
>      // If this origin's quota is being tracked (open handle to a database in this origin), add this new database
>      // to the quota manager now
>      String fullFilePath = SQLiteFileSystem::appendDatabaseFileNameToPath(originPath, fileName);
> -    if (originQuotaManagerNoLock().tracksOrigin(origin))
> -        originQuotaManagerNoLock().addDatabase(origin, name, fullFilePath);
> +    if (originQuotaManager().tracksOrigin(origin))
> +        originQuotaManager().addDatabase(origin, name, fullFilePath);
>  
>      return fullFilePath;
>  }
>  
>  String DatabaseTracker::fullPathForDatabase(SecurityOrigin* origin, const String& name, bool createIfNotExists)
>  {
> -    Locker<OriginQuotaManager> locker(originQuotaManager());
>      MutexLocker lockDatabase(m_databaseGuard);
> -    populateOrigins();
> +    Locker<OriginQuotaManager> quotaManagerLocker(originQuotaManager());
>  
>      return fullPathForDatabaseNoLock(origin, name, createIfNotExists).threadsafeCopy();
>  }
> @@ -297,8 +295,6 @@ void DatabaseTracker::populateOrigins()
>          return;
>  
>      m_quotaMap.set(new QuotaMap);
> -    if (!m_quotaManager)
> -        m_quotaManager.set(new OriginQuotaManager);
>  
>      openTrackerDatabase(false);
>      if (!m_database.isOpen())
> @@ -324,7 +320,6 @@ void DatabaseTracker::populateOrigins()
>  void DatabaseTracker::origins(Vector<RefPtr<SecurityOrigin> >& result)
>  {
>      MutexLocker lockDatabase(m_databaseGuard);
> -    populateOrigins();
>      ASSERT(m_quotaMap);
>      copyKeysToVector(*m_quotaMap, result);
>  }
> @@ -357,10 +352,12 @@ bool DatabaseTracker::databaseNamesForOr
>  
>  bool DatabaseTracker::databaseNamesForOrigin(SecurityOrigin* origin, Vector<String>& resultVector)
>  {
> -    MutexLocker lockDatabase(m_databaseGuard);
>      Vector<String> temp;
> -    if (!databaseNamesForOriginNoLock(origin, temp))
> -        return false;
> +    {
> +        MutexLocker lockDatabase(m_databaseGuard);
> +        if (!databaseNamesForOriginNoLock(origin, temp))
> +          return false;
> +    }
>  
>      for (Vector<String>::iterator iter = temp.begin(); iter != temp.end(); ++iter)
>          resultVector.append(iter->threadsafeCopy());
> @@ -504,8 +501,6 @@ void DatabaseTracker::removeOpenDatabase
>      if (!database)
>          return;
>  
> -    Locker<OriginQuotaManager> quotaManagerLocker(originQuotaManager());
> -    MutexLocker lockDatabase(m_databaseGuard);
>      MutexLocker openDatabaseMapLock(m_openDatabaseMapGuard);
>  
>      if (!m_openDatabaseMap) {
> @@ -541,7 +536,9 @@ void DatabaseTracker::removeOpenDatabase
>  
>      m_openDatabaseMap->remove(database->securityOrigin());
>      delete nameMap;
> -    originQuotaManagerNoLock().removeOrigin(database->securityOrigin());

You're now locking originQuotaManager after m_openDatabaseMapGuard [it used to have to go before], and the header file no longer specifies the ordering of those two locks.  If this code is right, please specify the ordering in the header file.  The reordering is probably doable, though, as m_openDatabaseMapGuard isn't used in many places.

> +    Locker<OriginQuotaManager> quotaManagerLocker(originQuotaManager());
> +    originQuotaManager().removeOrigin(database->securityOrigin());
>  }
>  
>  void DatabaseTracker::getOpenDatabases(SecurityOrigin* origin, const String& name, HashSet<RefPtr<Database> >* databases)
> @@ -564,30 +561,32 @@ void DatabaseTracker::getOpenDatabases(S
>  
>  unsigned long long DatabaseTracker::usageForOriginNoLock(SecurityOrigin* origin)
>  {
> -    ASSERT(!originQuotaManagerNoLock().tryLock());
> +    ASSERT(!originQuotaManager().tryLock());
>  
>      // Use the OriginQuotaManager mechanism to calculate the usage
> -    if (originQuotaManagerNoLock().tracksOrigin(origin))
> -        return originQuotaManagerNoLock().diskUsage(origin);
> +    if (originQuotaManager().tracksOrigin(origin))
> +        return originQuotaManager().diskUsage(origin);
>  
>      // If the OriginQuotaManager doesn't track this origin already, prime it to do so
> -    originQuotaManagerNoLock().trackOrigin(origin);
> +    originQuotaManager().trackOrigin(origin);
>  
>      Vector<String> names;
> -    databaseNamesForOriginNoLock(origin, names);
> +    {
> +        MutexLocker lockDatabase(m_databaseGuard);

You're taking a lock in a function whose name ends in NoLock.  Don't do that.  The whole point of the NoLock suffix is that you can trust it to mean something.  See if you can maintain an invariant of all locks being taken in public functions, and all functions called by DatabaseTracker methods being private NoLock functions.

> +        databaseNamesForOriginNoLock(origin, names);
> +    }
>  
>      for (unsigned i = 0; i < names.size(); ++i)
> -        originQuotaManagerNoLock().addDatabase(origin, names[i], fullPathForDatabaseNoLock(origin, names[i], false));
> +        originQuotaManager().addDatabase(origin, names[i], fullPathForDatabaseNoLock(origin, names[i], false));
>  
> -    if (!originQuotaManagerNoLock().tracksOrigin(origin))
> +    if (!originQuotaManager().tracksOrigin(origin))
>          return 0;
> -    return originQuotaManagerNoLock().diskUsage(origin);
> +    return originQuotaManager().diskUsage(origin);
>  }
>  
>  unsigned long long DatabaseTracker::usageForOrigin(SecurityOrigin* origin)
>  {
> -    Locker<OriginQuotaManager> locker(originQuotaManager());
> -
> +    Locker<OriginQuotaManager> quotaManagerLocker(originQuotaManager());
>      return usageForOriginNoLock(origin);
>  }
>  
> @@ -601,7 +600,6 @@ unsigned long long DatabaseTracker::quot
>  unsigned long long DatabaseTracker::quotaForOrigin(SecurityOrigin* origin)
>  {
>      MutexLocker lockDatabase(m_databaseGuard);
> -    populateOrigins();
>      return quotaForOriginNoLock(origin);
>  }
>  
> @@ -609,7 +607,6 @@ void DatabaseTracker::setQuota(SecurityO
>  {
>      MutexLocker lockDatabase(m_databaseGuard);
>  
> -    populateOrigins();
>      if (quotaForOriginNoLock(origin) == quota)
>          return;
>  
> @@ -721,13 +718,10 @@ void DatabaseTracker::deleteOrigin(Secur
>      }
>  
>      {
> -        // To satisfy the lock hierarchy, we have to lock the originQuotaManager before m_databaseGuard if there's any chance we'll to lock both.
> -        Locker<OriginQuotaManager> quotaManagerLocker(originQuotaManager());
>          MutexLocker lockDatabase(m_databaseGuard);
> -        SQLiteStatement statement(m_database, "DELETE FROM Databases WHERE origin=?");
> -
>          doneDeletingOrigin(origin);
>  
> +        SQLiteStatement statement(m_database, "DELETE FROM Databases WHERE origin=?");
>          if (statement.prepare() != SQLResultOk) {
>              LOG_ERROR("Unable to prepare deletion of databases from origin %s from tracker", origin->databaseIdentifier().ascii().data());
>              return;
> @@ -758,7 +752,10 @@ void DatabaseTracker::deleteOrigin(Secur
>          RefPtr<SecurityOrigin> originPossiblyLastReference = origin;
>          m_quotaMap->remove(origin);
>  
> -        originQuotaManagerNoLock().removeOrigin(origin);
> +        {
> +            Locker<OriginQuotaManager> quotaManagerLocker(originQuotaManager());
> +            originQuotaManager().removeOrigin(origin);
> +        }
>  
>          // If we removed the last origin, do some additional deletion.
>          if (m_quotaMap->isEmpty()) {
> @@ -912,8 +909,6 @@ void DatabaseTracker::deleteDatabase(Sec
>          return;
>      }
>  
> -    // To satisfy the lock hierarchy, we have to lock the originQuotaManager before m_databaseGuard if there's any chance we'll to lock both.
> -    Locker<OriginQuotaManager> quotaManagerLocker(originQuotaManager());
>      MutexLocker lockDatabase(m_databaseGuard);
>  
>      SQLiteStatement statement(m_database, "DELETE FROM Databases WHERE origin=? AND name=?");
> @@ -932,7 +927,10 @@ void DatabaseTracker::deleteDatabase(Sec
>          return;
>      }
>  
> -    originQuotaManagerNoLock().removeDatabase(origin, name);
> +    {
> +        Locker<OriginQuotaManager> quotaManagerLocker(originQuotaManager());
> +        originQuotaManager().removeDatabase(origin, name);
> +    }
>  
>      if (m_client) {
>          m_client->dispatchDidModifyOrigin(origin);
> @@ -951,8 +949,8 @@ bool DatabaseTracker::deleteDatabaseFile
>  
>  #ifndef NDEBUG
>      {
> -    MutexLocker lockDatabase(m_databaseGuard);
> -    ASSERT(deletingDatabase(origin, name) || deletingOrigin(origin));
> +        MutexLocker lockDatabase(m_databaseGuard);
> +        ASSERT(deletingDatabase(origin, name) || deletingOrigin(origin));

Accidental tabs.

>      }
>  #endif
>

[Eric U.]

> >      // If the OriginQuotaManager doesn't track this origin already, prime it to do so
> > -    originQuotaManagerNoLock().trackOrigin(origin);
> > +    originQuotaManager().trackOrigin(origin);
> >  
> >      Vector<String> names;
> > -    databaseNamesForOriginNoLock(origin, names);
> > +    {
> > +        MutexLocker lockDatabase(m_databaseGuard);
> 
> You're taking a lock in a function whose name ends in NoLock.  Don't do that. 
> The whole point of the NoLock suffix is that you can trust it to mean
> something.  See if you can maintain an invariant of all locks being taken in
> public functions, and all functions called by DatabaseTracker methods being
> private NoLock functions.

Also, this particular lock violates your new lock hierarchy, since it's m_databaseGuard after originQuotaManager.

[Brady Eidson]

Is this meant to be solely about refactoring?  No behavior changes?

[Dumitru Daniliuc]

Created attachment 52454 [details]
patch #1: remove the dependency on OriginQuotaManager from DatabaseTracker.h

Brady: this entire bug will be about refactoring. The idea is to keep in DatabaseTracker.h only the fields/methods used by all platforms and get rid of the #if !PLATFORM(CHROMIUM) conditionals.

(In reply to comment #5)
> (From update of attachment 52090 [details])
> > Index: WebCore/storage/DatabaseTracker.cpp
> > ===================================================================
> > --- WebCore/storage/DatabaseTracker.cpp	(revision 56810)
> > +++ WebCore/storage/DatabaseTracker.cpp	(working copy)
> > @@ -49,20 +49,13 @@
> >  
> >  using namespace std;
> >  
> > -namespace WebCore {
> > -
> > -OriginQuotaManager& DatabaseTracker::originQuotaManagerNoLock()
> > +static WebCore::OriginQuotaManager& originQuotaManager()
> >  {
> > -    ASSERT(m_quotaManager);
> > -    return *m_quotaManager;
> > +    DEFINE_STATIC_LOCAL(WebCore::OriginQuotaManager, quotaManager, ());
> 
> Does this have to be a static local?  Could we just create it in the
> DatabaseTracker constructor now?
> If it's always accessed from inside non-static DatabaseTracker methods, that
> should be simple.
> And then we don't even need the accessor function; it can just be a data
> member.

Chromium's DatabaseTracker implementation doesn't need this field, so I would like to remove it from DatabaseTracker.h. I removed the accessor method though: quotaManager is initialized in DatabaseTracker's constructor and destroyed in its destructor.

> > @@ -504,8 +501,6 @@ void DatabaseTracker::removeOpenDatabase
> >      if (!database)
> >          return;
> >  
> > -    Locker<OriginQuotaManager> quotaManagerLocker(originQuotaManager());
> > -    MutexLocker lockDatabase(m_databaseGuard);
> >      MutexLocker openDatabaseMapLock(m_openDatabaseMapGuard);
> >  
> >      if (!m_openDatabaseMap) {
> > @@ -541,7 +536,9 @@ void DatabaseTracker::removeOpenDatabase
> >  
> >      m_openDatabaseMap->remove(database->securityOrigin());
> >      delete nameMap;
> > -    originQuotaManagerNoLock().removeOrigin(database->securityOrigin());
> 
> You're now locking originQuotaManager after m_openDatabaseMapGuard [it used to
> have to go before], and the header file no longer specifies the ordering of
> those two locks.  If this code is right, please specify the ordering in the
> header file.  The reordering is probably doable, though, as
> m_openDatabaseMapGuard isn't used in many places.

Updated the expectations in DatabaseTracker.h. The new expectation for m_openDatabaseMapGuard is that the code locked on it should not acquire any other lock.

> >  unsigned long long DatabaseTracker::usageForOriginNoLock(SecurityOrigin* origin)
> >  {
> > -    ASSERT(!originQuotaManagerNoLock().tryLock());
> > +    ASSERT(!originQuotaManager().tryLock());
> >  
> >      // Use the OriginQuotaManager mechanism to calculate the usage
> > -    if (originQuotaManagerNoLock().tracksOrigin(origin))
> > -        return originQuotaManagerNoLock().diskUsage(origin);
> > +    if (originQuotaManager().tracksOrigin(origin))
> > +        return originQuotaManager().diskUsage(origin);
> >  
> >      // If the OriginQuotaManager doesn't track this origin already, prime it to do so
> > -    originQuotaManagerNoLock().trackOrigin(origin);
> > +    originQuotaManager().trackOrigin(origin);
> >  
> >      Vector<String> names;
> > -    databaseNamesForOriginNoLock(origin, names);
> > +    {
> > +        MutexLocker lockDatabase(m_databaseGuard);
> 
> You're taking a lock in a function whose name ends in NoLock.  Don't do that. 
> The whole point of the NoLock suffix is that you can trust it to mean
> something.  See if you can maintain an invariant of all locks being taken in
> public functions, and all functions called by DatabaseTracker methods being
> private NoLock functions.

Oops, fixed. Acquiring the lock on m_databaseGuard in usageForOrigin().

> >      {
> > -    MutexLocker lockDatabase(m_databaseGuard);
> > -    ASSERT(deletingDatabase(origin, name) || deletingOrigin(origin));
> > +        MutexLocker lockDatabase(m_databaseGuard);
> > +        ASSERT(deletingDatabase(origin, name) || deletingOrigin(origin));
> 
> Accidental tabs.

No, intentional. The code in this {} block should be indented by 4 spaces like all other {} blocks.

[Eric U.]

Comment on attachment 52454 [details]
patch #1: remove the dependency on OriginQuotaManager from DatabaseTracker.h

> Index: WebCore/storage/DatabaseTracker.h
> ===================================================================
> --- WebCore/storage/DatabaseTracker.h	(revision 57024)
> +++ WebCore/storage/DatabaseTracker.h	(working copy)
> @@ -52,7 +52,6 @@ struct SecurityOriginHash;
>  
>  #if !PLATFORM(CHROMIUM)
>  class DatabaseTrackerClient;
> -class OriginQuotaManager;
>  
>  struct SecurityOriginTraits;
>  #endif // !PLATFORM(CHROMIUM)
> @@ -62,8 +61,8 @@ public:
>      static DatabaseTracker& tracker();
>      // This singleton will potentially be used from multiple worker threads and the page's context thread simultaneously.  To keep this safe, it's
>      // currently using 4 locks.  In order to avoid deadlock when taking multiple locks, you must take them in the correct order:
> -    // originQuotaManager() before m_databaseGuard or m_openDatabaseMapGuard
> -    // m_databaseGuard before m_openDatabaseMapGuard
> +    // m_databaseGuard before quotaManager if both locks are needed.
> +    // no other lock is taken in the code locked on m_openDatabaseMapGuard.

I believe that m_openDatabaseMapGuard is actually independent of all other locks.  That is, we need not require that there's any ordering on it yet, just like with notificationMutex().

>      // notificationMutex() is currently independent of the other locks.
>  

LGTM other than that; now you need a real reviewer.

[Dumitru Daniliuc]

(In reply to comment #9)
> (From update of attachment 52454 [details])
> > Index: WebCore/storage/DatabaseTracker.h
> > ===================================================================
> > --- WebCore/storage/DatabaseTracker.h	(revision 57024)
> > +++ WebCore/storage/DatabaseTracker.h	(working copy)
> > @@ -52,7 +52,6 @@ struct SecurityOriginHash;
> >  
> >  #if !PLATFORM(CHROMIUM)
> >  class DatabaseTrackerClient;
> > -class OriginQuotaManager;
> >  
> >  struct SecurityOriginTraits;
> >  #endif // !PLATFORM(CHROMIUM)
> > @@ -62,8 +61,8 @@ public:
> >      static DatabaseTracker& tracker();
> >      // This singleton will potentially be used from multiple worker threads and the page's context thread simultaneously.  To keep this safe, it's
> >      // currently using 4 locks.  In order to avoid deadlock when taking multiple locks, you must take them in the correct order:
> > -    // originQuotaManager() before m_databaseGuard or m_openDatabaseMapGuard
> > -    // m_databaseGuard before m_openDatabaseMapGuard
> > +    // m_databaseGuard before quotaManager if both locks are needed.
> > +    // no other lock is taken in the code locked on m_openDatabaseMapGuard.
> 
> I believe that m_openDatabaseMapGuard is actually independent of all other
> locks.  That is, we need not require that there's any ordering on it yet, just
> like with notificationMutex().

Changed the comment for m_openDatabaseMapGuard to look similar to the comment for notificationMutex() (not uploading a new patch for this small change).

[Dimitri Glazkov (Google)]

Comment on attachment 52454 [details]
patch #1: remove the dependency on OriginQuotaManager from DatabaseTracker.h

r=me, except: 

>  
>  DatabaseTracker& DatabaseTracker::tracker()
>  {
> @@ -74,6 +63,16 @@ DatabaseTracker::DatabaseTracker()
>      : m_client(0)
>  {
>      SQLiteFileSystem::registerSQLiteVFS();
> +
> +    quotaManager = new OriginQuotaManager();

I think you should just do DEFINE_STATIC_LOCAL accessor, like you had in the previous patch. Tracker's lifetime is the same (it's also STATIC_LOCAL), so there's no need for trying to control its lifetime.

[Dumitru Daniliuc]

(In reply to comment #11)
> (From update of attachment 52454 [details])
> r=me, except: 
> 
> >  
> >  DatabaseTracker& DatabaseTracker::tracker()
> >  {
> > @@ -74,6 +63,16 @@ DatabaseTracker::DatabaseTracker()
> >      : m_client(0)
> >  {
> >      SQLiteFileSystem::registerSQLiteVFS();
> > +
> > +    quotaManager = new OriginQuotaManager();
> 
> I think you should just do DEFINE_STATIC_LOCAL accessor, like you had in the
> previous patch. Tracker's lifetime is the same (it's also STATIC_LOCAL), so
> there's no need for trying to control its lifetime.

done.

patch #1 landed as r57286.

[WebKit Review Bot]

http://trac.webkit.org/changeset/57286 might have broken Tiger Intel Release

[Eric Seidel (no email)]

This did break Tiger.  storage/open-database-creation-callback.html	 times out every time on the Tiger bot.

[Eric Seidel (no email)]

Any word on Tiger?  The bot has been broken for almost 3 hours. :(

[Eric Seidel (no email)]

Looks like the test may be flaky.

It's timed out on Snow Leopard as well:
http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r57290%20(7932)/results.html

[Dumitru Daniliuc]

(In reply to comment #16)
> Looks like the test may be flaky.
> 
> It's timed out on Snow Leopard as well:
> http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r57290%20(7932)/results.html

http://trac.webkit.org/changeset/57301 seems to have fix the problem on Snow Leopard. Waiting for Tiger to get to that patch.

[Eric Seidel (no email)]

Tiger failed again:
http://build.webkit.org/results/Tiger%20Intel%20Release/r57313%20(10590)/results.html

[Eric Seidel (no email)]

Snow Leopard failed again:
http://build.webkit.org/results/SnowLeopard%20Intel%20Leaks/r57313%20(5777)/results.html

[Eric Seidel (no email)]

12 hours later.  The bots are still red, the commit-queue is still blocked (the test times out on the commit-queue as well).  It seems time to roll this out.

[Eric Seidel (no email)]

I'm not quite sure what needs to be rolled out here.  I'm going to try with r57286 and r57301 and see if that makes the builders finally green again. :(

[Eric Seidel (no email)]

Looks like I should be able to get away with just rolling out r57286.  Doing so in bug 37312.

[Eric Seidel (no email)]

Comment on attachment 52454 [details]
patch #1: remove the dependency on OriginQuotaManager from DatabaseTracker.h

Marking r- since this was rolled out.

[Dumitru Daniliuc]

Created attachment 53399 [details]
 patch #1: remove the dependency on OriginQuotaManager from DatabaseTracker.h

Same patch, with a minor change that fixes a deadlock that caused some tests to fail.

[Dimitri Glazkov (Google)]

Comment on attachment 53399 [details]
 patch #1: remove the dependency on OriginQuotaManager from DatabaseTracker.h

it looks like you still have the quotaManager lifetime bound to that of DatabaseTracker instead of just making it local static.

Also, which was the change that you made to fix the test borkage?

[Dumitru Daniliuc]

Created attachment 53465 [details]
patch #1: remove the dependency on OriginQuotaManager from DatabaseTracker.h

(In reply to comment #25)
> (From update of attachment 53399 [details])
> it looks like you still have the quotaManager lifetime bound to that of
> DatabaseTracker instead of just making it local static.

Oops, sorry, it should be fixed now.

> Also, which was the change that you made to fix the test borkage?

I changed DatabaseTracker::getMaxSizeForDatabase(). this method used to lock on originQuotaManager(), and then it called quotaForOrigin(), which locked on m_databaseGuard. All other methods lock on m_databaseGuard first and then on originQuotaManager() (DatabaseTracker.h documents the lock ordering). So I changed getMaxSizeForDatabase() acquire locks in the correct order, and replaced quotaForOrigin() with quotaForOriginNoLock().

[Dimitri Glazkov (Google)]

Comment on attachment 53465 [details]
patch #1: remove the dependency on OriginQuotaManager from DatabaseTracker.h

ok.

[Dumitru Daniliuc]

Patch #1 landed as r57668.

[Eric Seidel (no email)]

Patch was landed, closing.  Please open new bugs for additional patches.