[Bug 31482] Refactor DatabaseTracker.h and eliminate all #ifdefs

[bugzill...@webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=31482

WebKit Review Bot <webkit.r...@gmail.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |webkit-bot-watchers@googleg
| |roups.com,
| |webkit.r...@gmail.com

--- Comment #4 from WebKit Review Bot <webkit.r...@gmail.com> 2010-03-30 15:42:23 PST ---
Attachment 52090 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.

--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.

[bugzill...@webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=31482





--- Comment #5 from Eric U. <er...@chromium.org> 2010-03-31 18:06:41 PST ---
(From update of attachment 52090)
> 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

[bugzill...@webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=31482

--- Comment #6 from Eric U. <er...@chromium.org> 2010-03-31 18:10:03 PST ---

> > // 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.

[bugzill...@webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=31482

--- Comment #7 from Brady Eidson <bei...@apple.com> 2010-03-31 19:49:05 PST ---
Is this meant to be solely about refactoring? No behavior changes?

[bugzill...@webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=31482


Dumitru Daniliuc <du...@chromium.org> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #52090|0 |1
is obsolete| |
Attachment #52090|review? |
Flag| |
Attachment #52454| |review?, commit-queue-
Flag| |




--- Comment #8 from Dumitru Daniliuc <du...@chromium.org> 2010-04-02 14:57:49 PST ---
Created an attachment (id=52454)
--> (https://bugs.webkit.org/attachment.cgi?id=52454)
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.

[bugzill...@webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=31482


Eric Seidel <er...@webkit.org> changed:

What |Removed |Added
----------------------------------------------------------------------------
Status|ASSIGNED |RESOLVED
Resolution| |FIXED




--- Comment #29 from Eric Seidel <er...@webkit.org> 2010-05-17 00:51:29 PST ---
Patch was landed, closing. Please open new bugs for additional patches.