Bug 30701 - Chromium needs a different implementation for DatabaseTracker (+ quota management) and SQLTransactionClient
Summary: Chromium needs a different implementation for DatabaseTracker (+ quota manage...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Dumitru Daniliuc
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-10-22 18:07 PDT by Dumitru Daniliuc
Modified: 2009-11-02 17:40 PST (History)
3 users (show)

See Also:


Attachments
patch (16.60 KB, patch)
2009-10-22 18:14 PDT, Dumitru Daniliuc
no flags Details | Formatted Diff | Diff
patch (15.98 KB, patch)
2009-10-29 13:10 PDT, Dumitru Daniliuc
no flags Details | Formatted Diff | Diff
patch (15.97 KB, patch)
2009-10-29 13:49 PDT, Dumitru Daniliuc
no flags Details | Formatted Diff | Diff
patch (16.28 KB, patch)
2009-10-30 13:01 PDT, Dumitru Daniliuc
no flags Details | Formatted Diff | Diff
patch (16.19 KB, patch)
2009-10-30 14:27 PDT, Dumitru Daniliuc
no flags Details | Formatted Diff | Diff
patch (15.93 KB, patch)
2009-11-02 15:19 PST, Dumitru Daniliuc
dglazkov: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dumitru Daniliuc 2009-10-22 18:07:56 PDT
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.
Comment 1 Dumitru Daniliuc 2009-10-22 18:14:54 PDT
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.
Comment 2 Michael Nordman 2009-10-29 00:05:17 PDT
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?
Comment 3 Michael Nordman 2009-10-29 09:27:33 PDT
86     String originIdentifier = database->databaseThreadSecurityOrigin()->databaseIdentifier();

Just database->securityOrigin() since that method returns the appropriate copy now.
Comment 4 Dumitru Daniliuc 2009-10-29 13:06:18 PDT
(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.
Comment 5 Dumitru Daniliuc 2009-10-29 13:10:20 PDT
Created attachment 42130 [details]
patch
Comment 6 Dumitru Daniliuc 2009-10-29 13:49:12 PDT
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.
Comment 7 Michael Nordman 2009-10-29 19:42:23 PDT
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;
}
Comment 8 Dumitru Daniliuc 2009-10-30 12:56:44 PDT
(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.
Comment 9 Dumitru Daniliuc 2009-10-30 13:01:36 PDT
Created attachment 42228 [details]
patch

Addressed Michael's comments.
Comment 10 Michael Nordman 2009-10-30 13:54:37 PDT
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.
Comment 11 Dumitru Daniliuc 2009-10-30 14:27:03 PDT
Created attachment 42234 [details]
patch

Oops, removed that line: originIdentifier is not used anywhere in that method anymore.
Comment 12 Michael Nordman 2009-10-30 15:35:28 PDT
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.
Comment 13 Dumitru Daniliuc 2009-10-30 16:22:52 PDT
(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.
Comment 14 Michael Nordman 2009-10-30 17:04:30 PDT
> 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
Comment 15 Dumitru Daniliuc 2009-11-02 14:21:08 PST
Ping. Looking for a WebKit reviewer.
Comment 16 Michael Nordman 2009-11-02 14:52:10 PST
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?
Comment 17 Dumitru Daniliuc 2009-11-02 15:19:31 PST
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.
Comment 18 Dimitri Glazkov (Google) 2009-11-02 15:56:38 PST
Comment on attachment 42345 [details]
patch

r=me.
Comment 19 Dumitru Daniliuc 2009-11-02 17:40:00 PST
Landed as r50434.