Bug 30701 – Chromium needs a different implementation for DatabaseTracker (+ quota management) and SQLTransactionClient

• Component: New Bugs
• Status: RESOLVED FIXED

[Dumitru Daniliuc]

We need to add Chromium specific implementations for DatabaseTracker (+ quota management) and SQLTransactionClient. WebCore's default implementation is not suitable because it's not modular enough for a multi-process application.

[Dumitru Daniliuc]

Created attachment 41708 [details]
patch

These new files will be added to WebCore.gypi in a future 2-sided patch that will put all pieces together.

[Michael Nordman]

This looks pretty good, some simple questions of the "just checking" nature.

12         * storage/chromium/DatabaseTrackerChromium.cpp: Added.
13         (WebCore::DatabaseTracker::tracker):
14         (WebCore::DatabaseTracker::DatabaseTracker):
15         (WebCore::DatabaseTracker::canEstablishDatabase):
16         (WebCore::DatabaseTracker::setDatabaseDetails):
17         (WebCore::DatabaseTracker::fullPathForDatabase):
18         (WebCore::DatabaseTracker::addOpenDatabase):
19         (WebCore::DatabaseTracker::removeOpenDatabase):
20         (WebCore::DatabaseTracker::getMaxSizeForDatabase):

In the ChangeLog, You don't need to list the modified methods for newly added files. Ditto for all of the added .cpp files.

 68 String DatabaseTracker::fullPathForDatabase(SecurityOrigin* origin, const String& name, bool)
 69 {
 70     return origin->databaseIdentifier() + "_" + name + ".db";
 71 }

nit: Should we lose the .db extension on these files? I don't think we're ready to retain storage of DBs in this system for all time quite yet, so not a real pressing issue. But what is the file layout *really*.

This code merely needs to return a string token that chrome can map to an actual file path in the file system in the browser process. What format should that string token take? I'd vote for something easily crackable into its constituent components <originIdentifier, name>.

73 void DatabaseTracker::addOpenDatabase(Database* database)
74 {
75     DatabaseObserver::databaseOpened(database);
76 }
77 
78 void DatabaseTracker::removeOpenDatabase(Database* database)
79 {
80     DatabaseObserver::databaseClosed(database);
81 }

Can/should you make any assertions about which thread these are being called on?

Also, in order to implement "purgeMemory()" should chromium's tracker maintain a collection of open databases, or does that happen elsewhere?


45     void getDatabaseSizeAndSpaceAvailableToOrigin(
46         const String& securityOrigin, const String& databaseName,
47         unsigned long long* databaseSize, unsigned long long* spaceAvailable);
48     void updateDatabaseSizeAndSpaceAvailableToOrigin(
49         const String& securityOrigin, const String& databaseName,
50         unsigned long long databaseSize, unsigned long long spaceAvailable);

maybe securityOriginIdentifier or originIdentifier for clarity?


51 void SQLTransactionClient::didCommitTransaction(SQLTransaction* transaction)
52 {
53     ASSERT(currentThread() == transaction->database()->document()->databaseThread()->getThreadID());
54     if (!transaction->isReadOnly())
55         callOnMainThread(notifyDatabaseChanged, transaction->database());
56 }

Are we guaranteed that 'database' is not finally released prior to the notifyDatabaseChanged running on the main thread?

[Michael Nordman]

86     String originIdentifier = database->databaseThreadSecurityOrigin()->databaseIdentifier();

Just database->securityOrigin() since that method returns the appropriate copy now.

[Dumitru Daniliuc]

(In reply to comment #2)
> This looks pretty good, some simple questions of the "just checking" nature.
> 
> 12         * storage/chromium/DatabaseTrackerChromium.cpp: Added.
> 13         (WebCore::DatabaseTracker::tracker):
> 14         (WebCore::DatabaseTracker::DatabaseTracker):
> 15         (WebCore::DatabaseTracker::canEstablishDatabase):
> 16         (WebCore::DatabaseTracker::setDatabaseDetails):
> 17         (WebCore::DatabaseTracker::fullPathForDatabase):
> 18         (WebCore::DatabaseTracker::addOpenDatabase):
> 19         (WebCore::DatabaseTracker::removeOpenDatabase):
> 20         (WebCore::DatabaseTracker::getMaxSizeForDatabase):
> 
> In the ChangeLog, You don't need to list the modified methods for newly added
> files. Ditto for all of the added .cpp files.

done.

>  68 String DatabaseTracker::fullPathForDatabase(SecurityOrigin* origin, const
> String& name, bool)
>  69 {
>  70     return origin->databaseIdentifier() + "_" + name + ".db";
>  71 }
> 
> nit: Should we lose the .db extension on these files? I don't think we're ready
> to retain storage of DBs in this system for all time quite yet, so not a real
> pressing issue. But what is the file layout *really*.
> 
> This code merely needs to return a string token that chrome can map to an
> actual file path in the file system in the browser process. What format should
> that string token take? I'd vote for something easily crackable into its
> constituent components <originIdentifier, name>.

this is the file name passed to the VFS layer, so it needs to match the file name we derive from <origin_identifier, database_name> in database_tracker in the browser process when we populate our cache of database sizes. eventually, we should change our IPCs coming from the VFS layer to pass a pair <origin_identifier, database_name> and have a centralized place that converts that to a file name; but for now, i think it needs to stay the way it is.

> 73 void DatabaseTracker::addOpenDatabase(Database* database)
> 74 {
> 75     DatabaseObserver::databaseOpened(database);
> 76 }
> 77 
> 78 void DatabaseTracker::removeOpenDatabase(Database* database)
> 79 {
> 80     DatabaseObserver::databaseClosed(database);
> 81 }
> 
> Can/should you make any assertions about which thread these are being called
> on?

done.

> Also, in order to implement "purgeMemory()" should chromium's tracker maintain
> a collection of open databases, or does that happen elsewhere?

added that set here.

> 45     void getDatabaseSizeAndSpaceAvailableToOrigin(
> 46         const String& securityOrigin, const String& databaseName,
> 47         unsigned long long* databaseSize, unsigned long long*
> spaceAvailable);
> 48     void updateDatabaseSizeAndSpaceAvailableToOrigin(
> 49         const String& securityOrigin, const String& databaseName,
> 50         unsigned long long databaseSize, unsigned long long spaceAvailable);
> 
> maybe securityOriginIdentifier or originIdentifier for clarity?

changed to originIdentifier;

> 51 void SQLTransactionClient::didCommitTransaction(SQLTransaction* transaction)
> 52 {
> 53     ASSERT(currentThread() ==
> transaction->database()->document()->databaseThread()->getThreadID());
> 54     if (!transaction->isReadOnly())
> 55         callOnMainThread(notifyDatabaseChanged, transaction->database());
> 56 }
> 
> Are we guaranteed that 'database' is not finally released prior to the
> notifyDatabaseChanged running on the main thread?

Database is RefCounted, so it seems to me that passing it to callOnMainThread() should increment the ref count, and it should get released at the end of notifyDatabaseChanged(). but i'm not sure...

> 86     String originIdentifier =
> database->databaseThreadSecurityOrigin()->databaseIdentifier();
> 
> Just database->securityOrigin() since that method returns the appropriate
> copy now.

fixed.

[Dumitru Daniliuc]

Created attachment 42130 [details]
patch

[Dumitru Daniliuc]

Created attachment 42135 [details]
patch

Same patch, only changed the file names returned by DatabaseTrackerChromium::fullPathForDatabase() from origin_identifier + "_" + db_name + ".db" to origin_identifier + "/" + db_name. We'll split that into origin_identifier and db_name in the browser process as we talked, and create the real file name there.

[Michael Nordman]

41 namespace {
42 
43 static void notifyDatabaseChanged(void* context) {
44     WebCore::DatabaseObserver::databaseModified(static_cast<WebCore::Database*>(context));
45 }
46 
47 }

Since the method is static, the anon namespace isn't needed... or vice versa.

------------------------
51 void SQLTransactionClient::didCommitTransaction(SQLTransaction* transaction)
52 {
53     ASSERT(currentThread() == transaction->database()->document()->databaseThread()->getThreadID());
54     if (!transaction->isReadOnly())
55         callOnMainThread(notifyDatabaseChanged, transaction->database());
56 }

Unless you're certain the database can't get finally released between the time the task is scheduled and the time the task executes, probably should ref()/deref() accordingly.

------------------------
46 static wtf::HashSet<Database*> openDatabases;

This collection needs to be wrapped up using DEFINE_STATIC_LOCAL. Also, I don't think you need to prefix with the 'wtf' namespace since HashSet.h contains using WTF::HashSet. (it looks like the namespace name is 'WTF', all caps, btw).

typedef HashSet<Database*> DatabaseSet;

static DatabaseSet& openDatabases()
{
    DEFINE_STATIC_LOCAL(DatabasesSet, staticOpenDatabases, ());
    return staticOpenDatabases;
}

[Dumitru Daniliuc]

(In reply to comment #7)
> 41 namespace {
> 42 
> 43 static void notifyDatabaseChanged(void* context) {
> 44    
> WebCore::DatabaseObserver::databaseModified(static_cast<WebCore::Database*>(context));
> 45 }
> 46 
> 47 }
> 
> Since the method is static, the anon namespace isn't needed... or vice versa.

removed the anonymous namespace.

> ------------------------
> 51 void SQLTransactionClient::didCommitTransaction(SQLTransaction* transaction)
> 52 {
> 53     ASSERT(currentThread() ==
> transaction->database()->document()->databaseThread()->getThreadID());
> 54     if (!transaction->isReadOnly())
> 55         callOnMainThread(notifyDatabaseChanged, transaction->database());
> 56 }
> 
> Unless you're certain the database can't get finally released between the time
> the task is scheduled and the time the task executes, probably should
> ref()/deref() accordingly.

ref()'ing in didCommitTransaction() and deref()'ing in notifyDatabaseChanged().

> ------------------------
> 46 static wtf::HashSet<Database*> openDatabases;
> 
> This collection needs to be wrapped up using DEFINE_STATIC_LOCAL. Also, I don't
> think you need to prefix with the 'wtf' namespace since HashSet.h contains
> using WTF::HashSet. (it looks like the namespace name is 'WTF', all caps, btw).
> 
> typedef HashSet<Database*> DatabaseSet;
> 
> static DatabaseSet& openDatabases()
> {
>     DEFINE_STATIC_LOCAL(DatabasesSet, staticOpenDatabases, ());
>     return staticOpenDatabases;
> }

done.

[Dumitru Daniliuc]

Created attachment 42228 [details]
patch

Addressed Michael's comments.

[Michael Nordman]

As mentioned before... i don't think this will compile as is...

86     String originIdentifier =
database->databaseThreadSecurityOrigin()->databaseIdentifier();

Just database->securityOrigin() since that method returns the appropriate copy
now.

[Dumitru Daniliuc]

Created attachment 42234 [details]
patch

Oops, removed that line: originIdentifier is not used anywhere in that method anymore.

[Michael Nordman]

I have one remaining question around string thread safety...

 59 void QuotaTracker::updateDatabaseSizeAndOriginSpace(
 60     const String& originIdentifier, const String& databaseName,
 61     unsigned long long databaseSize, unsigned long long spaceAvailable)
 62 {
 63     MutexLocker lockData(m_dataGuard);
 64     m_spaceAvailableToOrigins.set(originIdentifier, spaceAvailable);
 65     HashMap<String, SizeMap>::iterator it = m_databaseSizes.add(originIdentifier, SizeMap()).first;
 66     it->second.set(databaseName, databaseSize);
 67 }

... since these collections are used on accessed on multiple threads, what magic String incantations should we invoke on the string values being placed into these collections? Is crossThreadString() or threadsafeCopy() applicable to our situation here? Unless you know that answer to that question, please seek out advice from a webkit hacker on this point.

[Dumitru Daniliuc]

(In reply to comment #12)
> I have one remaining question around string thread safety...
> 
>  59 void QuotaTracker::updateDatabaseSizeAndOriginSpace(
>  60     const String& originIdentifier, const String& databaseName,
>  61     unsigned long long databaseSize, unsigned long long spaceAvailable)
>  62 {
>  63     MutexLocker lockData(m_dataGuard);
>  64     m_spaceAvailableToOrigins.set(originIdentifier, spaceAvailable);
>  65     HashMap<String, SizeMap>::iterator it =
> m_databaseSizes.add(originIdentifier, SizeMap()).first;
>  66     it->second.set(databaseName, databaseSize);
>  67 }
> 
> ... since these collections are used on accessed on multiple threads, what
> magic String incantations should we invoke on the string values being placed
> into these collections? Is crossThreadString() or threadsafeCopy() applicable
> to our situation here? Unless you know that answer to that question, please
> seek out advice from a webkit hacker on this point.

Looks like this should be OK. Strings are added to this map (as keys) only on the main thread, and the map is destroyed when the process goes away on the main thread too, and I don't see any ref()s/deref()s in the HashMap implementation, so I think we should be safe here. Jeremy and Dimitri seem to agree.

[Michael Nordman]

> Looks like this should be OK. Strings are added to this map (as keys) only on
> the main thread, and the map is destroyed when the process goes away on the
> main thread too, and I don't see any ref()s/deref()s in the HashMap
> implementation, so I think we should be safe here. Jeremy and Dimitri seem to
> agree.

Great... fwiw... lgtm

[Dumitru Daniliuc]

Ping. Looking for a WebKit reviewer.

[Michael Nordman]

46 typedef HashSet<Database*> DatabaseSet;
47 
48 static DatabaseSet& openDatabases()
49 {
50     DEFINE_STATIC_LOCAL(DatabasesSet, staticOpenDatabases, ());
51     return staticOpenDatabases;
52 }

Since it looks like peter doesn't need this to get his job done, are you planning to remove this?

[Dumitru Daniliuc]

Created attachment 42345 [details]
patch

(In reply to comment #16)
> 46 typedef HashSet<Database*> DatabaseSet;
> 47 
> 48 static DatabaseSet& openDatabases()
> 49 {
> 50     DEFINE_STATIC_LOCAL(DatabasesSet, staticOpenDatabases, ());
> 51     return staticOpenDatabases;
> 52 }
> 
> Since it looks like peter doesn't need this to get his job done, are you
> planning to remove this?

done.

[Dimitri Glazkov (Google)]

Comment on attachment 42345 [details]
patch

r=me.

[Dumitru Daniliuc]

Landed as r50434.