Bug 27967 – Decouple the code that deals with the main DB and quota management from the rest of the DB code

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

[Dumitru Daniliuc]

In Chromium, we want to read/write to the main DB only in the browser process. This will make sure that a malicious renderer cannot corrupt the main DB and will not be able to "discover" all existing databases in the system. We also need to move the quota management code to the browser process because it needs to keep track of what's going on in all renderer processes.

We intend to do this in a few steps:
1. Add a "listener" interface for SQLTransaction. This would allow us to remove SQLTransaction's dependencies on DatabaseTracker and OriginQuotaManager, as well as allow us to "listen" to different SQLTransaction stages more easily in the future, if needed.
2. Abstract out DatabaseTracker into an interface, and make all DB classes rely on the interface only. The code that's currently in DatabaseTracker.cpp would be WebCore's implementation of this interface.
3. Add the Chromium-specific implementations.

Patches for each step to come...

[Dumitru Daniliuc]

Created attachment 34104 [details]
SQLTransactionClient

Introducing SQLTransactionClient, a "listener" interface that allows us to remove dependencies on DatabaseTracker and OriginQuotaManager from SQLTransaction.

[Dimitri Glazkov (Google)]

Comment on attachment 34104 [details]
SQLTransactionClient

I think you should just name SQLTransactionClient implementation SQLTransactionClient.cpp. There's no need to do an extra suffix. If you intend to provide a separate implementation in Chromium,
you can provide it in platform/chromium and name it SQLTransactionClientChromium.cpp.

Overall, the idea sounds great!

> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog	(revision 46789)
> +++ WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,47 @@
> [phone]  Dumitru Daniliuc  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Added a "listener" interface to SQLTransaction that allows us to

You can just say a client to SQLTransaction. It's a common term here.

>  				>
>  			</File>
>  			<File
> +				RelativePath="..\storage\SQLTransactionClient.h"
> +				>
> +			</File>
> +			<File
> +				RelativePath="..\storage\SQLTransactionClientImpl.cpp"
> +				>
> +			</File>
> +			<File
>  				RelativePath="..\storage\SQLTransactionErrorCallback.h"
>  				>
>  			</File>


You will need to make changes to all build systems that currently use SQLTransaction.

> Index: WebCore/storage/Database.cpp
> ===================================================================
> --- WebCore/storage/Database.cpp	(revision 46789)
> +++ WebCore/storage/Database.cpp	(working copy)
> @@ -50,6 +50,7 @@
>  #include "SQLiteFileSystem.h"
>  #include "SQLiteStatement.h"
>  #include "SQLResultSet.h"
> +#include "SQLTransactionClient.h"
>  #include <wtf/MainThread.h>
>  #endif
>  
> @@ -136,6 +137,7 @@ Database::Database(Document* document, c
>      , m_deleted(false)
>      , m_stopped(false)
>      , m_opened(false)
> +    , m_transactionClient(new SQLTransactionClient())

What's the point of a client if you don't pass it in as an argument?

> Index: WebCore/storage/SQLTransactionClientImpl.cpp
> ===================================================================
> +namespace WebCore {
> +
> +void SQLTransactionClient::databaseChanged(Database* database) {
> +  DatabaseTracker::tracker().scheduleNotifyDatabaseChanged(database->securityOriginCopy().get(), database->stringIdentifier());

4 spaces, brace on new line.

> +void SQLTransactionClient::databaseSizeChanged(Database* database) {

brace on new line.

> +bool SQLTransactionClient::databaseExceedsQuota(Database* database) {

brace on new line.

[Dumitru Daniliuc]

Created attachment 34296 [details]
patch

(In reply to comment #2)

reverted all clean up changes. opened bug 28075 for that.

> > Index: WebCore/ChangeLog
> > ===================================================================
> > --- WebCore/ChangeLog	(revision 46789)
> > +++ WebCore/ChangeLog	(working copy)
> > @@ -1,3 +1,47 @@
> > [phone]  Dumitru Daniliuc  <[e-mail]>
> > +
> > +        Reviewed by NOBODY (OOPS!).
> > +
> > +        Added a "listener" interface to SQLTransaction that allows us to
> 
> You can just say a client to SQLTransaction. It's a common term here.

done.

> >  				>
> >  			</File>
> >  			<File
> > +				RelativePath="..\storage\SQLTransactionClient.h"
> > +				>
> > +			</File>
> > +			<File
> > +				RelativePath="..\storage\SQLTransactionClientImpl.cpp"
> > +				>
> > +			</File>
> > +			<File
> >  				RelativePath="..\storage\SQLTransactionErrorCallback.h"
> >  				>
> >  			</File>
> 
> 
> You will need to make changes to all build systems that currently use
> SQLTransaction.

d'oh! added.

> > Index: WebCore/storage/Database.cpp
> > ===================================================================
> > --- WebCore/storage/Database.cpp	(revision 46789)
> > +++ WebCore/storage/Database.cpp	(working copy)
> > @@ -50,6 +50,7 @@
> >  #include "SQLiteFileSystem.h"
> >  #include "SQLiteStatement.h"
> >  #include "SQLResultSet.h"
> > +#include "SQLTransactionClient.h"
> >  #include <wtf/MainThread.h>
> >  #endif
> >  
> > @@ -136,6 +137,7 @@ Database::Database(Document* document, c
> >      , m_deleted(false)
> >      , m_stopped(false)
> >      , m_opened(false)
> > +    , m_transactionClient(new SQLTransactionClient())
> 
> What's the point of a client if you don't pass it in as an argument?

where would you pass it as an argument? i was thinking that we would have one default client. if somebody wants to monitor a transaction in more details (s)he would add more hooks/code to the default client. and if other ports (like Chromium) want a completely different functionality, they could just provide their own implementation and not include the default SQLTransaction.cpp file in their build. what are your thoughts on this?

> > Index: WebCore/storage/SQLTransactionClientImpl.cpp
> > ===================================================================
> > +namespace WebCore {
> > +
> > +void SQLTransactionClient::databaseChanged(Database* database) {
> > +  DatabaseTracker::tracker().scheduleNotifyDatabaseChanged(database->securityOriginCopy().get(), database->stringIdentifier());
> 
> 4 spaces, brace on new line.

fixed.

> > +void SQLTransactionClient::databaseSizeChanged(Database* database) {
> 
> brace on new line.

fixed.

> > +bool SQLTransactionClient::databaseExceedsQuota(Database* database) {
> 
> brace on new line.

fixed.

[Eric Seidel (no email)]

Comment on attachment 34296 [details]
patch

I need more background in the ChangeLog here.  Why are we doing this?  So that Chromium can have the TransactionClient in a separate process?

0 seems like the wrong default here:
 71                                              PassRefPtr<VoidCallback>, PassRefPtr<SQLTransactionWrapper>, SQLTransactionClient* transactionClient = 0);

I would think that we would want transactions to "work out of the box" for webkit clients.  it seems possible here to construct a transaction which would not work correctly out of the box!

Style:
 48 void SQLTransactionClient::databaseSizeChanged(Database* database) {

Why is a copy needed here?
 59     RefPtr<SecurityOrigin> origin = database->securityOriginCopy();

In general the chagnge looks fine.  I'm concerned about the =0 default.  I also need more background in teh ChangeLog about why we're doing this (I'm not opposed to it, but the ChangeLog needs to document our reasoning.)

[Dumitru Daniliuc]

(In reply to comment #4)
> (From update of attachment 34296 [details])
> I need more background in the ChangeLog here.

Added some of the following explanation to ChangeLog.

> Why are we doing this?  So that Chromium can have the TransactionClient in
> a separate process?

Not exactly. SQLTransactionClient is supposed to have 2 functions:

1. (Not important for us at this point) Be a centralized place for getting notifications about certain events in a transaction.
2. (The main reason we're doing this) Provide an abstraction layer that allows each WebKit port to define how transactions interact with the main DB. For example, in Chromium we want to access the main DB only in the browser process.* So while WebCore's default implementation makes direct calls to DatabaseTracker, Chromium's implementation will send IPCs to the browser process.

* First, there's a security concern: we don't want a malicious renderer to be able to corrupt the main DB. And second, the main DB stores a bit of state (like per-origin quota) that needs to be synchronized across all renderers.

> 0 seems like the wrong default here:
>  71                                              PassRefPtr<VoidCallback>,
> PassRefPtr<SQLTransactionWrapper>, SQLTransactionClient* transactionClient =
> 0);
> 
> I would think that we would want transactions to "work out of the box" for
> webkit clients.  it seems possible here to construct a transaction which
> would not work correctly out of the box!

removed the default.

> Style:
>  48 void SQLTransactionClient::databaseSizeChanged(Database* database) {

fixed.

> Why is a copy needed here?
>  59     RefPtr<SecurityOrigin> origin = database->securityOriginCopy();

it seems to be the only way to get the origin for a database from a Database object.

[Dumitru Daniliuc]

Created attachment 34338 [details]
patch

[Michael Nordman]

File: WebCore/storage/SQLTransaction.cpp

329             if (m_transactionClient)
392     if (m_transactionClient)
426     if (m_modifiedDatabase && m_transactionClient)

There are a few tests for m_transactionClient being null in this file. Are those really needed, are there any situations where the client is expected to be null? (It looks like there aren't). And if there are no valid situations for a null client... maybe ASSERT(transClient) in the ctor.

File: WebCore/ChangeLog
34         * storage/SQLTransactionClient.cpp: Added.
35         (WebCore::SQLTransactionClient::databaseChanged):
36         (WebCore::SQLTransactionClient::databaseSizeChanged):
37         (WebCore::SQLTransactionClient::databaseExceedsQuota):

I think when adding new files, the preference is to not list all of the newly added methods. For changes to existing files, its useful to see specifically which methods changed, but not so for newly added files.

[Dumitru Daniliuc]

Created attachment 34488 [details]
patch

(In reply to comment #7)
> File: WebCore/storage/SQLTransaction.cpp
> 
> 329             if (m_transactionClient)
> 392     if (m_transactionClient)
> 426     if (m_modifiedDatabase && m_transactionClient)
> 
> There are a few tests for m_transactionClient being null in this file. Are
> those really needed, are there any situations where the client is expected to
> be null? (It looks like there aren't). And if there are no valid situations for
> a null client... maybe ASSERT(transClient) in the ctor.

done.

> File: WebCore/ChangeLog
> 34         * storage/SQLTransactionClient.cpp: Added.
> 35         (WebCore::SQLTransactionClient::databaseChanged):
> 36         (WebCore::SQLTransactionClient::databaseSizeChanged):
> 37         (WebCore::SQLTransactionClient::databaseExceedsQuota):
> 
> I think when adding new files, the preference is to not list all of the newly
> added methods. For changes to existing files, its useful to see specifically
> which methods changed, but not so for newly added files.

done.

[Dumitru Daniliuc]

Created attachment 34496 [details]
patch

Same patch, fixed a typo.

[Dumitru Daniliuc]

Created attachment 34617 [details]
patch

Same patch, SQLTransactionClient's methods are no longer virtual as suggested by Michael.

[Michael Nordman]

187             'storage/SQLTransactionClientImpl.cpp',
Some of the make files make reference to SQLTransactionClientImpl.cpp, did you mean SQLTransactionClient.cpp here?

330     m_transactionClient->databaseSizeChanged(m_database.get());
So this method is invoked on the background db thread. 

392     m_shouldRetryCurrentStatement = m_transactionClient->databaseExceedsQuota(...);
And this one on the 'apartment' thread of the Database object (the main thread)

426     m_transactionClient->databaseChanged(m_database.get());
And this one on the background thread

I would appreciate some indication of the threading usage in the SQLTransactionListener source files. To figure out how it was used, i had to track down the callsites. Not sure how to best indicate the threading rules... either doc comments in the .h file, or runtime asserts in the .cpp file?

38     // A "listener" interface to the SQLTransaction class. Allows
 39     // SQLTransaction to notify interested parties when certain things are
 40     // happening in a transaction.
 41     class SQLTransactionClient {
 42     public:
 43         // Notifies everybody that the database has changed. Called at the end
 44         // end of every successful transaction.
 45         void databaseChanged(Database*);
 46 
 47         // Notifies everybody that the size of the database file might have changed.
 48         // Called after executing each statement in a transaction
 49         void databaseSizeChanged(Database*);
 50 
 51         // Notifies everybody that the size of the database file exceeds its quota.
 52         // If this method returns true, the current statement in the transaction is
 53         // retried. Otherwise, the transaction is rolled back.
 54         //
 55         // This method should return true if and only if the quota for this database
 56         // file was increased (by the user, for example) since calling this method.
 57         bool databaseExceedsQuota(Database*);
 58     };
 59 }

After reading your doc comments, maybe these methods could be renamed for clarity.

void didCommitTransaction(...);  // for databaseChanged
void didExecuteStatement(...);  // for databaseSizeChanged
bool didExceedQuota(...);  // for databaseExceedsQuota

And would it make sense to have the SQLTransaction* be the parameter since it is a transaction listener, the caller can get the Database* from the accessor on the transaction object (i think).

Also, the new class (TransactionCoordinator) you just added is also a transaction listener of sorts. Should these two classes be merged? The TransactionCoordinator must be owned by the DBThread to properly serve its purpose. In this patch, you have the new 'listener' object being owned by the Database object, but i think it could just as easily be owned by the DBThread and serve its purpose.

Wdyt?

60     RefPtr<SecurityOrigin> origin = database->securityOriginCopy();

Making a copy of a refcounted object looks odd? Is it mutable? Is there no database->securityOrigin() accessor?

[Dumitru Daniliuc]

Created attachment 34873 [details]
patch

(In reply to comment #11)
> 187             'storage/SQLTransactionClientImpl.cpp',
> Some of the make files make reference to SQLTransactionClientImpl.cpp, did you
> mean SQLTransactionClient.cpp here?

yes, fixed.

> 330     m_transactionClient->databaseSizeChanged(m_database.get());
> So this method is invoked on the background db thread. 
> 
> 392     m_shouldRetryCurrentStatement =
> m_transactionClient->databaseExceedsQuota(...);
> And this one on the 'apartment' thread of the Database object (the main thread)
> 
> 426     m_transactionClient->databaseChanged(m_database.get());
> And this one on the background thread
> 
> I would appreciate some indication of the threading usage in the
> SQLTransactionListener source files. To figure out how it was used, i had to
> track down the callsites. Not sure how to best indicate the threading rules...
> either doc comments in the .h file, or runtime asserts in the .cpp file?

added ASSERTs in the .cpp file. i don't think we should make assumptions for all ports.

> 38     // A "listener" interface to the SQLTransaction class. Allows
>  39     // SQLTransaction to notify interested parties when certain things are
>  40     // happening in a transaction.
>  41     class SQLTransactionClient {
>  42     public:
>  43         // Notifies everybody that the database has changed. Called at the
> end
>  44         // end of every successful transaction.
>  45         void databaseChanged(Database*);
>  46 
>  47         // Notifies everybody that the size of the database file might have
> changed.
>  48         // Called after executing each statement in a transaction
>  49         void databaseSizeChanged(Database*);
>  50 
>  51         // Notifies everybody that the size of the database file exceeds
> its quota.
>  52         // If this method returns true, the current statement in the
> transaction is
>  53         // retried. Otherwise, the transaction is rolled back.
>  54         //
>  55         // This method should return true if and only if the quota for this
> database
>  56         // file was increased (by the user, for example) since calling this
> method.
>  57         bool databaseExceedsQuota(Database*);
>  58     };
>  59 }
> 
> After reading your doc comments, maybe these methods could be renamed for
> clarity.
> 
> void didCommitTransaction(...);  // for databaseChanged
> void didExecuteStatement(...);  // for databaseSizeChanged
> bool didExceedQuota(...);  // for databaseExceedsQuota

changed.

> And would it make sense to have the SQLTransaction* be the parameter since it
> is a transaction listener, the caller can get the Database* from the accessor
> on the transaction object (i think).

done. you're right: SQLTransactionCoordinator should work with SQLTransactions.

> Also, the new class (TransactionCoordinator) you just added is also a
> transaction listener of sorts. Should these two classes be merged?

i think we will eventually merge them. for now though, i think it's better to introduce SQLTransactionClient as a separate piece of code (to make it easier to review and understand).

> The TransactionCoordinator must be owned by the DBThread to properly serve its
> purpose. In this patch, you have the new 'listener' object being owned by the
> Database object, but i think it could just as easily be owned by the DBThread
> and serve its purpose.
> 
> Wdyt?

done.

> 60     RefPtr<SecurityOrigin> origin = database->securityOriginCopy();
> 
> Making a copy of a refcounted object looks odd? Is it mutable? Is there no
> database->securityOrigin() accessor?

this seems to be the only accessor for getting the origin from a Database object. we could probably introduce a new accessor (or replace the existing one), but i wouldn't want to do this before talking to the authors of this code and understanding why they had a copying accessor here in the first place.

[Michael Nordman]

(In reply to comment #12)

This lgtm, but you need a webkit reviewer.

> added ASSERTs in the .cpp file

Thank you.

> > void didCommitTransaction(...);  // for databaseChanged
> > void didExecuteStatement(...);  // for databaseSizeChanged
> > bool didExceedQuota(...);  // for databaseExceedsQuota
> 
> changed.

Ah... much more webkitty ;)

> this seems to be the only accessor for getting the origin from a Database
> object. we could probably introduce a new accessor (or replace the existing
> one), but i wouldn't want to do this before talking to the authors of this code
> and understanding why they had a copying accessor here in the first place.

I see, it looks like this may have to do with thread safety. The class is refcounted but not a thread safe refcount.

[Dimitri Glazkov (Google)]

Comment on attachment 34873 [details]
patch

Looks good. A few non-blocking comments below.

> +        * GNUmakefile.am:
> +        * WebCore.gypi:
> +        * WebCore.vcproj/WebCore.vcproj:
> +        * WebCore.xcodeproj/project.pbxproj:

What about WebCore.pro and other build systems? I am assuming you grepped through all of them to make sure we won't introduce some port breakage here.

> +        // Notifies everybody that the transaction was successfully commited.
> +        void didCommitTransaction(SQLTransaction*);

I think "Notifies everybody that" is superfluous here and below.

> +
> +        // Notifies everybody that a statement in the given transaction has been executed.
> +        void didExecuteStatement(SQLTransaction*);
> +
> +        // Notifies everybody that the size of the database file exceeds its quota.
> +        // If this method returns true, the current statement in the transaction is
> +        // retried. Otherwise, the transaction is rolled back.
> +        bool didExceedQuota(SQLTransaction*);

[Dumitru Daniliuc]

Created attachment 35052 [details]
patch

Same patch, addressed Dimitri's comments.

[Dumitru Daniliuc]

Created attachment 35054 [details]
patch

Forgot to add some build files to ChangeLog.

[Dimitri Glazkov (Google)]

Comment on attachment 35054 [details]
patch

r=me.

[Eric Seidel (no email)]

Comment on attachment 35054 [details]
patch

Rejecting patch 35054 from commit-queue.  This patch will require manual commit.

Failed to run "['git', 'svn', 'rebase']"  exit_code: 1  cwd: None

[Eric Seidel (no email)]

Comment on attachment 35054 [details]
patch

Sorry, we hit https://bugs.webkit.org/show_bug.cgi?id=28436

[Eric Seidel (no email)]

Comment on attachment 35054 [details]
patch

Rejecting patch 35054 from commit-queue.  This patch will require manual commit.

Patch https://bugs.webkit.org/attachment.cgi?id=35054 from bug 27967 failed to download and apply.

[Eric Seidel (no email)]

patching file WebCore/WebCore.xcodeproj/project.pbxproj
Hunk #2 succeeded at 8770 (offset 1 line).
Hunk #3 succeeded at 9879 (offset 2 lines).
Hunk #4 FAILED at 17320.
Hunk #5 succeeded at 19389 (offset 4 lines).
1 out of 5 hunks FAILED -- saving rejects to file WebCore/WebCore.xcodeproj/project.pbxproj.rej
patch -p0 "WebCore/WebCore.xcodeproj/project.pbxproj" returned 1.  Pass --force to ignore patch failures.

[Dumitru Daniliuc]

Created attachment 35082 [details]
resolved patch

[Dimitri Glazkov (Google)]

Comment on attachment 35082 [details]
resolved patch

carrying over the r+ flag.

[Eric Seidel (no email)]

Comment on attachment 35082 [details]
resolved patch

Rejecting patch 35082 from commit-queue.  This patch will require manual commit.

['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1'] failed with exit code 1

[Eric Seidel (no email)]

Running tests from /Users/eseidel/Projects/WebKit2/LayoutTests
Testing 11101 test cases.
fast/profiler/compare-multiple-profiles.html -> crashed

Maybe that test is just flakey, not sure.

[Dumitru Daniliuc]

(In reply to comment #25)
> Running tests from /Users/eseidel/Projects/WebKit2/LayoutTests
> Testing 11101 test cases.
> fast/profiler/compare-multiple-profiles.html -> crashed
> 
> Maybe that test is just flakey, not sure.

Must be a flaky test, and I don't think it's related to storage/DB in any way.

[Eric Seidel (no email)]

Comment on attachment 35082 [details]
resolved patch

On the theory that the test is flakey, adding this back to the commit queue.

[Eric Seidel (no email)]

Comment on attachment 35082 [details]
resolved patch

Clearing flags on attachment: 35082

Committed r47477: <http://trac.webkit.org/changeset/47477>

[Eric Seidel (no email)]

All reviewed patches have been landed.  Closing bug.

[Mark Rowe (bdash)]

Rolled out in r47481 as this caused 10 regression tests to hit assertion failures on the debug bots and 1 test to crash on the release bots.  See <http://build.webkit.org/results/Leopard%20Intel%20Debug%20(Tests)/r47477%20(4150)/results.html> and <http://build.webkit.org/results/Leopard%20Intel%20Release%20(Tests)/r47479%20(4175)/results.html>for info.

[Eric Seidel (no email)]

Hum.  It must have at least passed the tests in release mode to get out of teh commit-queue.  I could easily make the commit-queue build in debug too, but running both the debug and release tests before landing would be super slow. :)

[Dumitru Daniliuc]

Created attachment 35150 [details]
patch

The problem was that we used a RefPtr copy of a SecurityOrigin (that was destroyed as soon as we were out of its scope) instead of using the original pointer in SQLTransactionClient::didCommitTransaction().

We should probably add a Database::securityOrigin() method and explain in more details when to use securityOrigin() and when to use securityOriginCopy(). Right now, it is not obvious at all what the difference is, and it's very tempting to call m_database->securityOriginCopy() instead of m_database->document()->securityOrigin().

[Michael Nordman]

(In reply to comment #32)
> The problem was that we used a RefPtr copy of a SecurityOrigin (that was
> destroyed as soon as we were out of its scope) instead of using the original
> pointer in SQLTransactionClient::didCommitTransaction().

Right... so we traded a threading bug for a refcounting bug... ooops.

I wish bugzilla let me view just the diffs between the previous patch and this new one? Just did so with a text diff tool on the old an new patch files.

Your change lgtm.

> We should probably add a Database::securityOrigin() method and explain in more
> details when to use securityOrigin() and when to use securityOriginCopy().
> Right now, it is not obvious at all what the difference is, and it's very
> tempting to call m_database->securityOriginCopy() instead of
> m_database->document()->securityOrigin().

Agreed that something could be done to improve on this. SecurityOrigin has a thread safe refcount, but the underlying object is not thread safe. This is a fragile situation? I think think addressing that situation is probably out-of-scope for this fix.

[Adam Barth]

--- Comment #44 from Jeremy Orlow <[e-mail]>  2009-08-20 18:44:02 PDT ---
Committed r47612: <http://trac.webkit.org/changeset/47612>

[Jeremy Orlow]

Note this was landed in 47612 but the final posting of the patch was lost due to db corruption.

[Eric Seidel (no email)]

Comment on attachment 35150 [details]
patch

Landed.