Bug 40607 – Implement the sync DB API in workers

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

[Dumitru Daniliuc]

Implement the sync DB API in workers. If there's another bug opened for this already, please merge them -- I couldn't find any.

[Dumitru Daniliuc]

Created attachment 58744 [details]
patch #1: implement DatabaseSync::openDatabaseSync()

All new code in DatabaseSync.cpp was copy-pasted from Database.cpp (with a couple of trivial changes, such as removing calls to DatabaseThread).

The plan is to:
1. Implement DatabaseSync.cpp one "feature" at a time (openDatabaseSync(), transaction(), clean up everything properly when the worker shuts down in the middle of a DB operation, etc.)
2. Copy-paste code from Database.cpp to DatabaseSync.cpp and modify it as needed, in order to make each feature in DatabaseSync.cpp work.
3. Once feature 'n' is landed in DatabaseSync.cpp, work on feature 'n + 1', and in parallel work on moving the duplicate code introduced in feature 'n' to AbstractDatabase.

I think the advantages of this approach are:
1. We _know_ what code is duplicated when moving it from Database/DatabaseSync to AbstractDatabase, instead of guessing.
2. Moving duplicate code to AbstractDatabase doesn't block the DatabaseSync implementation; instead, it can be done in parallel.

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/storage/DatabaseSync.cpp:343:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 1 in 6 files


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

[Dumitru Daniliuc]

> WebCore/storage/DatabaseSync.cpp:343:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]

This code is copy-pasted from Database.cpp. I think it might be easier to review this patch if the new code in DatabaseSync matches exactly the existing one in Database. I can fix this style problem when moving this common code to AbstractDatabase.

[Dumitru Daniliuc]

Created attachment 58972 [details]
patch #2: Add the SQLException class

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/bindings/js/JSExceptionBase.cpp:39:  One or more unexpected \r (^M) found; better to use only a \n  [whitespace/carriage_return] [1]
Suppressing further [whitespace/carriage_return] reports for this file.
Total errors found: 7 in 20 files


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

[WebKit Review Bot]

Attachment 58972 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3278278

[Dumitru Daniliuc]

Created attachment 59039 [details]
patch #2: Add the SQLException class

Same patch, converted line endings in JSExceptionBase.cpp from CRLF to LF.

The Chromium bot fails because apparently it misses a step that regenerates .h/.cpp files from .idl files. I tried the patch in a local Chromium client and had no issues with it.

[WebKit Review Bot]

Attachment 59039 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3295338

[Adam Barth]

Comment on attachment 59039 [details]
patch #2: Add the SQLException class

WebCore/bindings/js/JSExceptionBase.cpp:65
 +          return reinterpret_cast<ExceptionBase*>(pathException);
Why does this diff look so funny?

WebCore/page/DOMWindow.idl:733
 +          attribute SQLExceptionConstructor SQLException;
Do you need to update the results of the tests that enumerate the global object for all the ports?

WebCore/storage/SQLException.h:7
 +   * version 2 of the License, or (at your option) any later version.
2.0?  I thought we used 2.1..  Google usually contributes under BSD.

[Adam Barth]

Did you run the tests?  That patch is likely to need updated expectations for some.

[Dumitru Daniliuc]

(In reply to comment #9)
> (From update of attachment 59039 [details])
> WebCore/bindings/js/JSExceptionBase.cpp:65
>  +          return reinterpret_cast<ExceptionBase*>(pathException);
> Why does this diff look so funny?

JSExceptionBase.cpp currently uses CRLF line endings, and I had to convert them all to LF to make the style checker happy.

Working on the other comments.

[Dumitru Daniliuc]

Created attachment 59095 [details]
patch #2: Add the SQLException class

patch #2 should be ready for another look.

Fixed the copyright header.

Fixed the expectations of the failing tests on Mac + the expectations for those tests on all other ports that I could find.

The patch passes all tests on Mac; a whole bunch of tests fail in the Qt port, but none of them seems related to this patch (most of them are in css1/, editing/ and fast/dom/Geolocation/ -- maybe I didn't pass in some flags?).

[WebKit Review Bot]

Attachment 59095 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3266343

[Adam Barth]

Comment on attachment 59095 [details]
patch #2: Add the SQLException class

Adding things to the global scope is a bit of a pain.  Please watch the bots carefully to make sure you've got all the expectation updates.

WebCore/WebCore.xcodeproj/project.pbxproj:11037
 +  				81CC114011BEAA9D00D0D856 /* IDBKeyRange.idl */,
Did you mean to add these files?

[Dumitru Daniliuc]

> WebCore/WebCore.xcodeproj/project.pbxproj:11037
>  +                  81CC114011BEAA9D00D0D856 /* IDBKeyRange.idl */,
> Did you mean to add these files?

No. Hmm, not sure how they got there. Thanks for catching this!

[Dumitru Daniliuc]

patch #2 landed as r61531.

[WebKit Review Bot]

http://trac.webkit.org/changeset/61531 might have broken GTK Linux 64-bit Debug

[Dumitru Daniliuc]

(In reply to comment #17)
> http://trac.webkit.org/changeset/61531 might have broken GTK Linux 64-bit Debug

Missed the GTK expectations for 2 tests, should be fixed now.

[Adam Roben (:aroben)]

Comment on attachment 59095 [details]
patch #2: Add the SQLException class

This change caused fast/dom/prototype-inheritance.html to fail on Windows. New results need to be landed. See http://build.webkit.org/results/Windows%20Debug%20(Tests)/r61558%20(15282)/results.html

[Dumitru Daniliuc]

(In reply to comment #19)
> (From update of attachment 59095 [details])
> This change caused fast/dom/prototype-inheritance.html to fail on Windows. New results need to be landed. See http://build.webkit.org/results/Windows%20Debug%20(Tests)/r61558%20(15282)/results.html

Updated the expectations in r61570.

[Dumitru Daniliuc]

Created attachment 59320 [details]
patch #1: implement DatabaseSync::openDatabaseSync()

Michael, I changed patch #1 as we discussed (I think...). Basically, I moved performOpenAndVerify() + some fields + simple getters for those fields from Database to AbstractDatabase. I also moved all guid-related methods to AbstractDatabase, but the plan is to move them to a separate class once this patch is landed. I also made a trivial change to DatabaseTracker, in order to remove the dependency on Database. I haven't changed the logic at all.

I can already see a few ways to clean up Database further, but I'll work on those in separate patches.

All tests in storage/ passed.

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/storage/AbstractDatabase.cpp:334:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
WebCore/storage/DatabaseAuthorizer.h:34:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/storage/Database.cpp:477:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 3 in 12 files


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

[Dumitru Daniliuc]

> Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
> WebCore/storage/AbstractDatabase.cpp:334:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
> WebCore/storage/DatabaseAuthorizer.h:34:  Alphabetical sorting problem.  [build/include_order] [4]
> WebCore/storage/Database.cpp:477:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> Total errors found: 3 in 12 files

Fix all style problems.

[Michael Nordman]

> void DatabaseSync::markAsDeletedAndClose()
> {
>     // FIXME: we probably need to do something else in here other than calling closeImmediately().
>     closeImmediately();
> }
> 
> void DatabaseSync::closeImmediately()
> {
>     // FIXME: probably need to do more things in here
>     closeDatabase();
>     DatabaseTracker::tracker().removeOpenDatabase(this);
> }

These methods are not called on the ScriptExecutionContext's thread, but the closeDatabase() method uses the sqlite3 database handle. That looks like a threading violation that needs to be resolved.

[Eric Seidel (no email)]

Comment on attachment 59039 [details]
patch #2: Add the SQLException class

Cleared Adam Barth's review+ from obsolete attachment 59039 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Dumitru Daniliuc]

Created attachment 59556 [details]
patch #1: implement DatabaseSync::openDatabaseSync()

(In reply to comment #24)
> > void DatabaseSync::markAsDeletedAndClose()
> > {
> >     // FIXME: we probably need to do something else in here other than calling closeImmediately().
> >     closeImmediately();
> > }
> > 
> > void DatabaseSync::closeImmediately()
> > {
> >     // FIXME: probably need to do more things in here
> >     closeDatabase();
> >     DatabaseTracker::tracker().removeOpenDatabase(this);
> > }
> 
> These methods are not called on the ScriptExecutionContext's thread, but the closeDatabase() method uses the sqlite3 database handle. That looks like a threading violation that needs to be resolved.

fixed.

[Dumitru Daniliuc]

Created attachment 59566 [details]
patch #1: implement DatabaseSync::openDatabaseSync()

[Dumitru Daniliuc]

Created attachment 59573 [details]
patch #1: implement DatabaseSync::openDatabaseSync()

[Michael Nordman]

This refactoring LGTM. Mostly moving code, only altering what needs altering to be shared with the sync case.

> Implementing DatabaseSync::openDatabaseSync().

The CL description could use some word smithing to briefly describe the changes made in support of implementing openDatabaseSync().

* Hoisted many methods involved in opening a web database from Database to AbstractDatabase for reuse in DatabaseSync.
* Virtualized performOpenAndVerify() to keep interactions with the DatabaseThread out of the base class as that only applies to the Database class and not DatabaseSync.
* Avoided storing a reference to the creation callback in the base class since its not needed beyond return from the static constructor method in the sync case.

[Michael Nordman]

Oooops... I spoke a little too some on the code part being ready. Couple of things to look at.

* In the usual case where a DatabaseSync is not closed forcibly thru closeImmediately() or markAsDeletedAndClose(), looks like we're leaving dangling pointers in the 'tracker' and cruft in the various GUID map goo maintained by the base class.

* The FIXME comments on closeImmediately() and markAsDeletedAndClose() are vague. Please be more clear about what has to happen or not.

I think you've got a good impl of closeImmediately() in place already and can simply remove the FIXME in that method.

I haven't understood the semantics of markAsDeletedAndClose() until about an hour ago. Similar semantics to closeImmediately() except its expected to be done in a sync fashion and there are additional side effects around the version() method (and around the callers of the public deleted() method). The impl in the most recent patch is not correct since it doesn't behave syncly. I think its OK to leave this undone for now, and just put in a FIXME for it.

[Dumitru Daniliuc]

Created attachment 59696 [details]
patch #1: implement DatabaseSync::openDatabaseSync()

Addressed Michael's comments.

[Michael Nordman]

Comment on attachment 59696 [details]
patch #1: implement DatabaseSync::openDatabaseSync()

> +void DatabaseSync::closeImmediately()
>  {
> -    ASSERT(m_scriptExecutionContext->isContextThread());
> -    return m_scriptExecutionContext.get();
> +    if (!opened())
> +        return;

I think you should move the call to opened() down so that the method is called on the context thread.

> +
> +    if (!m_scriptExecutionContext->isContextThread()) {
> +        m_scriptExecutionContext->postTask(CloseSyncDatabaseOnContextThreadTask::create(this));
> +        return;
> +    }
> +
> +    DatabaseTracker::tracker().removeOpenDatabase(this);
> +
> +    closeDatabase();
>  }


> DatabaseSync::~DatabaseSync()
> {
>     ASSERT(m_scriptExecutionContext->isContextThread());
> +    closeImmediately();
> }

Oh, thats a virtual method. Probably want to avoid calling that from the dtor.
http://www.artima.com/cppsource/nevercall.html

A seperate, private, non-virtual DatabaseSync::closeAndRemoveFromTracker() method thats only be called on the context thread may help that can be called from the virtual closeImmediately and from the dtor.

[Dumitru Daniliuc]

Created attachment 59713 [details]
patch #1: implement DatabaseSync::openDatabaseSync()

(In reply to comment #32)
> (From update of attachment 59696 [details])
> > +void DatabaseSync::closeImmediately()
> >  {
> > -    ASSERT(m_scriptExecutionContext->isContextThread());
> > -    return m_scriptExecutionContext.get();
> > +    if (!opened())
> > +        return;
> 
> I think you should move the call to opened() down so that the method is called on the context thread.

done.

> > DatabaseSync::~DatabaseSync()
> > {
> >     ASSERT(m_scriptExecutionContext->isContextThread());
> > +    closeImmediately();
> > }
> 
> Oh, thats a virtual method. Probably want to avoid calling that from the dtor.
> http://www.artima.com/cppsource/nevercall.html

gotta love C++... i changed the call to closeImmediately() to:

if (opened()) {
    DatabaseTracker.tracker().removeOpenDatabase(this);
    closeDatabase();
}

[Michael Nordman]

lgtm!

[Adam Barth]

Comment on attachment 59713 [details]
patch #1: implement DatabaseSync::openDatabaseSync()

This a pretty big patch to make sure all the code is getting move around properly, but the patch appears correct.

WebCore/storage/AbstractDatabase.cpp:137
 +      if (origin.endsWith("/"))
I still don't understand how origin can ever end with a /

WebCore/storage/AbstractDatabase.cpp:390
 +      DEFINE_STATIC_LOCAL(String, setVersionQuery, ("INSERT INTO " + databaseInfoTableName() + " (key, value) VALUES ('" + databaseVersionKey() + "', ?);"));
Does this mean that this table name is visible to web content?  That would be unfortunate.

[Dumitru Daniliuc]

(In reply to comment #35)
> (From update of attachment 59713 [details])
> This a pretty big patch to make sure all the code is getting move around properly, but the patch appears correct.
> 
> WebCore/storage/AbstractDatabase.cpp:137
>  +      if (origin.endsWith("/"))
> I still don't understand how origin can ever end with a /

Techinically, SecurityOrigin::toString() can return "file://". However, I don't think it matters: we just need a string that uniquely identifies the origin + DB name, and adding another '/' in case of "file://" doesn't change that. So I removed the check and changed this code to always use origin + '/' + name as a unique DB identifier.

> WebCore/storage/AbstractDatabase.cpp:390
>  +      DEFINE_STATIC_LOCAL(String, setVersionQuery, ("INSERT INTO " + databaseInfoTableName() + " (key, value) VALUES ('" + databaseVersionKey() + "', ?);"));
> Does this mean that this table name is visible to web content?  That would be unfortunate.

It's not. DatabaseAuthorizer doesn't allow the web app to do anything on this table (not even read it).

[Dumitru Daniliuc]

patch #1 landed as r61812.

[Dumitru Daniliuc]

Created attachment 59831 [details]
patch #1: implement DatabaseSync::openDatabaseSync()

Same patch, with minor changes:
1. Made ScriptExecutionContext aware of sync DBs, because we need a place to close them before the destructor runs (calling DatabaseTracker::removeOpenDatabase() in the destructor leads to weird things in Chromium).
2. Because of #1, moved 2 sync DB-specific lines from the loop in ScriptExecutionContext::stopDatabases() to Database::stop().
3. Commented out some test cases in the layout test, because they fail in V8. The failure happens because unlike JSC, v8 suppresses the errors in toString(). I'm investigating why this is so, and what can be done to make JSC and V8 behave the same way.

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/ChangeLog:13:  Line contains tab character.  [whitespace/tab] [5]
WebCore/ChangeLog:15:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 2 in 16 files


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

[Dumitru Daniliuc]

> WebCore/ChangeLog:13:  Line contains tab character.  [whitespace/tab] [5]
> WebCore/ChangeLog:15:  Line contains tab character.  [whitespace/tab] [5]
> Total errors found: 2 in 16 files

Will fix these on landing or when uploading a new version of the patch.

[Dumitru Daniliuc]

Created attachment 59833 [details]
patch #1: implement DatabaseSync::openDatabaseSync()

[Eric Seidel (no email)]

Comment on attachment 59713 [details]
patch #1: implement DatabaseSync::openDatabaseSync()

Cleared Adam Barth's review+ from obsolete attachment 59713 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Michael Nordman]

Comment on attachment 59833 [details]
patch #1: implement DatabaseSync::openDatabaseSync()

I see where ScriptExecutionContext's collection of open databases has been modified to accept DatabaseSync instances as well as Database instances, but I don't see where the DatabaseSync instances are ever added/removed to/from that collection. Maybe i'm missing something, could you walk me thru how that happens?

[Dumitru Daniliuc]

Created attachment 60066 [details]
patch #1: implement DatabaseSync::openDatabaseSync()

Should be the final version...

[Michael Nordman]

Comment on attachment 60066 [details]
patch #1: implement DatabaseSync::openDatabaseSync()

lgtm (the ChangeLog description should be modified since 4 and 5 no longer apply)

> +        Reviewed by NOBODY (OOPS!).
> +
> +        Implementing DatabaseSync::openDatabaseSync().
> +        https://bugs.webkit.org/show_bug.cgi?id=40607
> +
> +        1. Moved some common code from Database to AbstractDatabase.
> +        2. Made performOpenAndVerify() virtual, since DatabaseSync doesn't
> +           need to interact with DatabaseThread.
> +        3. Removed the m_creationCallback field, since it's only needed in
> +           the openDatabase{Sync} methods.
> +        4. Made ScriptExecutionContext aware of sync DBs, so we can close
> +           the sync DBs before the destructor is called.
> +        5. Moved a couple of async DB-specific lines from
> +           ScriptExecutionContext::stopDatabases() to Database::stop().

[Dumitru Daniliuc]

> lgtm (the ChangeLog description should be modified since 4 and 5 no longer apply)
> 
> > +        Reviewed by NOBODY (OOPS!).
> > +
> > +        Implementing DatabaseSync::openDatabaseSync().
> > +        https://bugs.webkit.org/show_bug.cgi?id=40607
> > +
> > +        1. Moved some common code from Database to AbstractDatabase.
> > +        2. Made performOpenAndVerify() virtual, since DatabaseSync doesn't
> > +           need to interact with DatabaseThread.
> > +        3. Removed the m_creationCallback field, since it's only needed in
> > +           the openDatabase{Sync} methods.
> > +        4. Made ScriptExecutionContext aware of sync DBs, so we can close
> > +           the sync DBs before the destructor is called.
> > +        5. Moved a couple of async DB-specific lines from
> > +           ScriptExecutionContext::stopDatabases() to Database::stop().

oops, will do on landing.

[Dumitru Daniliuc]

patch #1 re-landed as r62153.

[Dumitru Daniliuc]

Created attachment 60325 [details]
patch #3: implementing DatabaseSync::transaction() and changeVersion() + tests

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/storage/chromium/SQLTransactionSyncClientChromium.cpp:35:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 65 files


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

[Dumitru Daniliuc]

> WebCore/storage/chromium/SQLTransactionSyncClientChromium.cpp:35:  Alphabetical sorting problem.  [build/include_order] [4]

Fixed. Will wait for more comments before I upload a new patch.

[WebKit Review Bot]

Attachment 60325 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3349346

[Dumitru Daniliuc]

Created attachment 60412 [details]
patch #3: implementing DatabaseSync::transaction() and changeVersion() + tests

Same patch, should fix the style problem, and should build on Chromium.

[WebKit Review Bot]

Attachment 60412 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3408082

[Dumitru Daniliuc]

Created attachment 60445 [details]
patch #3: implementing DatabaseSync::transaction() and changeVersion() + tests

Addressed the problems Michael and I talked about offline:
1. An attempt to run a transaction inside another transaction should immediately result in an exception being thrown; added a test for that too.
2. Removed the SQLTransactionSyncClient class, and changed SQLTransactionClient to be usable by both sync and async transactions.
3. Changed SQLStatementSync.cpp to be a 'svn copy' of SQLStatement.cpp.
4. Replaced 'typedef int ExceptionCode' with '#include "ExceptionCode.h"' in all DB header files.
5. Changed the name of the SQLTransactionClient::didCommitTransaction() method to didCommitWriteTransaction().

I also changed '#include <wtf/Threading.h>' to '#include <wtf/ThreadSafeShared.h>' in a few callbacks, removed a few unnecessary #includes in SQLTransactionClientChromium.cpp, and moved the code that runs a transaction to a separate DatabaseSync::runTransaction() method, so it can be shared by DatabaseSync::transaction() and DatabaseSync::changeVersion().

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/storage/SQLTransactionSync.cpp:39:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 77 files


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

[Dumitru Daniliuc]

> Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
> WebCore/storage/SQLTransactionSync.cpp:39:  Alphabetical sorting problem.  [build/include_order] [4]
> Total errors found: 1 in 77 files

Fixed, will wait for more comments before re-uploading the patch.

[Adam Barth]

Comment on attachment 60445 [details]
patch #3: implementing DatabaseSync::transaction() and changeVersion() + tests

This patch is huge.  I only reviewed the first half of it and there's already some concerning stuff.  :(

WebCore/storage/DatabaseSync.cpp:95
 +          return new ChangeVersionPreflightStep(expectedVersion);
AdoptRef ???  This looks like a leak.

WebCore/storage/DatabaseSync.cpp:115
 +      ChangeVersionPreflightStep(const String& expectedVersion) : m_expectedVersion(expectedVersion) { }
explicit?  Please make this more than one line.

WebCore/storage/DatabaseSync.cpp:123
 +          return new ChangeVersionPostflightStep(newVersion);
adoptRef.  If you haven't already, please read http://webkit.org/coding/RefPtr.html

WebCore/storage/DatabaseSync.cpp:138
 +      ChangeVersionPostflightStep(const String& newVersion) : m_newVersion(newVersion) { }
explicit, multiline.

WebCore/storage/DatabaseSync.cpp:162
 +      if (!transaction->begin(ec) || !transaction->execute(ec) || !transaction->commit(ec))
Can these return ec and also true?  If not, that seems redundant.

WebCore/storage/SQLStatementSync.cpp:27
 +   */
missing space

WebCore/storage/SQLStatementSync.cpp:59
 +  bool SQLStatement::execute(Database* db)
This function is too complicated for me to understand at 4am.

WebCore/storage/SQLStatementSync.cpp:2
 +   * Copyright (C) 2010 Google Inc. All rights reserved.
Woah there.  You can't just change the copyright on a file from Apple to Google.  What's going on here with the license block?

WebCore/storage/SQLStatementSync.cpp:75
 +      for (unsigned int i = 0; i < m_arguments.size(); ++i) {
We usually just say "unsigned"

WebCore/storage/SQLStatementSync.h:54
 +  
Extra blank line.

WebCore/storage/SQLTransaction.h: 
 +  typedef int ExceptionCode;
Who review this line of code?  It's nonsense.  (Hope it wasn't me!)

WebCore/storage/SQLTransactionClient.h:44
 +  class SQLTransactionClient : public Noncopyable {
If this is a client, why aren't the methods virtual?

WebCore/storage/SQLTransactionSync.cpp:80
 +  PassRefPtr<SQLResultSet> SQLTransactionSync::executeSQL(const String& sqlStatement, const Vector<SQLValue>& arguments, ExceptionCode& ec)
again, too big for my little brain.

[Dumitru Daniliuc]

Created attachment 60812 [details]
patch #3: implementing DatabaseSync::transaction() and changeVersion() + tests

> This patch is huge.

the layout tests are probably more than half of it. another good chunk of it is changes to build files.

> I only reviewed the first half of it and there's already some concerning stuff.  :(

happy to fix them. :)

> WebCore/storage/DatabaseSync.cpp:95
>  +          return new ChangeVersionPreflightStep(expectedVersion);
> AdoptRef ???  This looks like a leak.

fixed.

> WebCore/storage/DatabaseSync.cpp:115
>  +      ChangeVersionPreflightStep(const String& expectedVersion) : m_expectedVersion(expectedVersion) { }
> explicit?  Please make this more than one line.

done.

> WebCore/storage/DatabaseSync.cpp:123
>  +          return new ChangeVersionPostflightStep(newVersion);
> adoptRef.  If you haven't already, please read http://webkit.org/coding/RefPtr.html

done. i thought PassRefPtr::PassRefPtr(T*) and adoptRef() did the same thing.

> WebCore/storage/DatabaseSync.cpp:138
>  +      ChangeVersionPostflightStep(const String& newVersion) : m_newVersion(newVersion) { }
> explicit, multiline.

done.

> WebCore/storage/DatabaseSync.cpp:162
>  +      if (!transaction->begin(ec) || !transaction->execute(ec) || !transaction->commit(ec))
> Can these return ec and also true?  If not, that seems redundant.

you're right, they are redundant. fixed, made these functions return an ExceptionCode.

> WebCore/storage/SQLStatementSync.cpp:27
>  +   */
> missing space

not sure what you mean, didn't change anything.

> WebCore/storage/SQLStatementSync.cpp:59
>  +  bool SQLStatement::execute(Database* db)
> This function is too complicated for me to understand at 4am.

SQLStatement{Sync}::execute() do the following:
1. changes the authorizer mode to read-only, if it's a read-only transaction (to make sure write-statements are disallowed).
2. prepares the statement.
3. makes sure the number of arguments matches the number of '?'s in the statements.
4. bind all arguments.
5. step through the statement, and add data to the result set, one row at a time.

> WebCore/storage/SQLStatementSync.cpp:2
>  +   * Copyright (C) 2010 Google Inc. All rights reserved.
> Woah there.  You can't just change the copyright on a file from Apple to Google.  What's going on here with the license block?

oops, reverted, and added a 1-line google copyright. also fixed the copyright notice in all other sync classes (they should all have the google copyright header).

> WebCore/storage/SQLStatementSync.cpp:75
>  +      for (unsigned int i = 0; i < m_arguments.size(); ++i) {
> We usually just say "unsigned"

fixed.

> WebCore/storage/SQLStatementSync.h:54
>  +  
> Extra blank line.

removed.

> WebCore/storage/SQLTransaction.h: 
>  +  typedef int ExceptionCode;
> Who review this line of code?  It's nonsense.  (Hope it wasn't me!)

it was around since... forever. michael had the same comment, so i removed the 'typedef int ExceptionCode;' line from all DB header files and added '#include "ExceptionCode.h"'.

> WebCore/storage/SQLTransactionClient.h:44
>  +  class SQLTransactionClient : public Noncopyable {
> If this is a client, why aren't the methods virtual?

made them virtual, even though this is the only implementation we have (well, 1 implementation for chromium, and 1 for all other ports), and i think it's quite unlikely we'll ever have another one.

> WebCore/storage/SQLTransactionSync.cpp:80
>  +  PassRefPtr<SQLResultSet> SQLTransactionSync::executeSQL(const String& sqlStatement, const Vector<SQLValue>& arguments, ExceptionCode& ec)
> again, too big for my little brain.

same code as SQLTransaction::executeSQL(), only instead of storing a SQLError, we set a SQLException.

[Darin Adler]

"typedef int ExceptionCode" was historically the standard way to “forward-declare” the ExceptionCode type, so we didn’t have to include ExceptionCode.h in every single header.

The idea was that header contained the actual exception code constants, but if you only needed the WebCore::ExceptionCode type you could typedef it to int. Any files that included both your header and ExceptionCode.h would end up checking it for you. If the typedef didn't match the compiler would generate an error.

Still-older code simply used int, which I felt was less self-documenting.

It’s fine to just include ExceptionCode.h in more places and get rid of that old usage, but it was standard at one time.

[Eric Seidel (no email)]

Attachment 60812 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/3448001

[Dumitru Daniliuc]

Created attachment 60817 [details]
patch #3: implementing DatabaseSync::transaction() and changeVersion() + tests

Same patch, should build on Mac.

[Adam Barth]

> It’s fine to just include ExceptionCode.h in more places and get rid of that old usage, but it was standard at one time.

Thanks, I didn't know that.  It seems suboptimal to duplicate that declaration everywhere.  I'm glad we're moving away from that pattern.

[Michael Nordman]

Comment on attachment 60817 [details]
patch #3: implementing DatabaseSync::transaction() and changeVersion() + tests

This is a big patch, I haven't looked at the test cases yet, but I am glad that you have put together a bunch of them to verify that this code works.

WebCore/storage/DatabaseSync.cpp:159
 +      RefPtr<SQLTransactionSync> transaction = transactionPrP;
Do you need to local variable here? Seems like deref'ing the prp is all you need.

WebCore/storage/SQLStatementSync.cpp:122
 +      return resultSet.release();
Can you just return resultSet here the local var is going out of scope momentarily. I'm not really sure which is preferred usage in webkit, please check.

WebCore/storage/SQLTransactionClient.h:49
 +      virtual bool didExceedQuota(AbstractDatabase*);
These really don't have to be virtual. It's not the type of 'client' interface that's is designed to be overriden. Perhaps using the term 'client' was a poor choice in webkit? If the need ever arises we can make them virtual at that time..

WebCore/storage/SQLTransactionSync.cpp:142
 +          ExceptionCode preflightExceptionCode = m_preflightStep->performStep(this);
This pre/post flight mechanism is used to read/write the version stored in the database. Shouldn't we be doing these reads and writes in the transaction?

Are there any other use cases for the pre/post stuff besides the version change use case? It may be overkill for just this use case. Instead of using runTransaction(), the changeVersion() method could be a slightly modified form of runTransaction() essentially.

void changeVersion(...) {
  trans.begin();
  read version from db and compare with expected;
  trans.execute(); // invoke the user callback 
  write new version to db;
  trans.commit();
}

wdyt?

WebCore/storage/SQLTransactionSync.cpp:208
 +      return postflightExceptionCode;
Ditto shouldn't we be doing this in the transaction.

[David Levin]

(In reply to comment #63) 
> WebCore/storage/SQLStatementSync.cpp:122
>  +      return resultSet.release();
> Can you just return resultSet here the local var is going out of scope momentarily. I'm not really sure which is preferred usage in webkit, please check.

fwiw, the way it is written is the preferred usage.

"If a function’s result is a new object or ownership is being transferred for any other reason, the result should be a PassRefPtr. Since local variables are typically RefPtr, it’s common to call release in the return statement to transfer the RefPtr to the PassRefPtr." -- http://webkit.org/coding/RefPtr.html

[Darin Adler]

(In reply to comment #64)
> (In reply to comment #63) 
> > WebCore/storage/SQLStatementSync.cpp:122
> >  +      return resultSet.release();
> > Can you just return resultSet here the local var is going out of scope momentarily. I'm not really sure which is preferred usage in webkit, please check.
> 
> fwiw, the way it is written is the preferred usage.
> 
> "If a function’s result is a new object or ownership is being transferred for any other reason, the result should be a PassRefPtr. Since local variables are typically RefPtr, it’s common to call release in the return statement to transfer the RefPtr to the PassRefPtr." -- http://webkit.org/coding/RefPtr.html

Yes, the release() is preferred.

If you forget to do the release(), then the reference count gets incremented and then decremented. If you remember to do the release(), then the reference count isn't touched. So it's a bit more efficient if you do the release(). There’s no correctness problem either way.

[Dumitru Daniliuc]

> WebCore/storage/DatabaseSync.cpp:159
>  +      RefPtr<SQLTransactionSync> transaction = transactionPrP;
> Do you need to local variable here? Seems like deref'ing the prp is all you need.

removed the runTransaction() method, so this statement went away.

> WebCore/storage/SQLStatementSync.cpp:122
>  +      return resultSet.release();
> Can you just return resultSet here the local var is going out of scope momentarily. I'm not really sure which is preferred usage in webkit, please check.

as Dave and Darin have said, this should be the right way to do this.

> WebCore/storage/SQLTransactionClient.h:49
>  +      virtual bool didExceedQuota(AbstractDatabase*);
> These really don't have to be virtual. It's not the type of 'client' interface that's is designed to be overriden. Perhaps using the term 'client' was a poor choice in webkit? If the need ever arises we can make them virtual at that time..

reverted. i'll see if adam has any objection to this.

> WebCore/storage/SQLTransactionSync.cpp:142
>  +          ExceptionCode preflightExceptionCode = m_preflightStep->performStep(this);
> This pre/post flight mechanism is used to read/write the version stored in the database. Shouldn't we be doing these reads and writes in the transaction?

i think the spec contradicts itself: http://dev.w3.org/html5/webdatabase/#dom-database-sync-changeversion. on one hand, it says that check version + invoke callback + update version should be done atomically. on the other hand, if you look at the step by step description, it looks to me like things should be done in this order:

1. begin transaction
2. check version
3. invoke callback
4. commit transaction
5. update version

i think doing it atomically is the right way to do it, so in the code i moved the "check version" and "update version" steps inside the transaction. i'll send an email to hixie and ask him for a clarification (and we should probably make it more clear in the spec too).

> Are there any other use cases for the pre/post stuff besides the version change use case? It may be overkill for just this use case. Instead of using runTransaction(), the changeVersion() method could be a slightly modified form of runTransaction() essentially.
> 
> void changeVersion(...) {
>   trans.begin();
>   read version from db and compare with expected;
>   trans.execute(); // invoke the user callback 
>   write new version to db;
>   trans.commit();
> }
> 
> wdyt?

done. initially i thought that "optional transaction steps" might be cool, but now i'm thinking that it's quite unlikely that we'll ever need to use them for anything else.

> WebCore/storage/SQLTransactionSync.cpp:208
>  +      return postflightExceptionCode;
> Ditto shouldn't we be doing this in the transaction.

done.

[Dumitru Daniliuc]

Created attachment 61015 [details]
patch #3: implementing DatabaseSync::transaction() and changeVersion() + tests

[Eric Seidel (no email)]

Attachment 61015 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/3499062

[Dumitru Daniliuc]

Created attachment 61025 [details]
patch #3: implementing DatabaseSync::transaction() and changeVersion() + tests

Same patch, should make gcc happy this time.

[Michael Nordman]

Comment on attachment 61025 [details]
patch #3: implementing DatabaseSync::transaction() and changeVersion() + tests

I think the db logic looks correct and the new classes are nicely composed. How the SQLTransactionClient class is used is kind of funky. A stateless object but we're making global static instance.

51 static SQLTransactionClient* transactionClient()
52 {
53     DEFINE_STATIC_LOCAL(SQLTransactionClient, transactionClient, ());
54     return &transactionClient;
55 }

I don't think DEFINE_STATIC_LOCAL construction macro is thread safe and this getter will definitely be called on different threads. It may be better to just add an SQLTransactionClient data member to SQLTransactionSync and call the methods thru that instance.

[Michael Nordman]

otherwise, i think this looks good

[Dumitru Daniliuc]

Created attachment 61134 [details]
patch #3: implementing DatabaseSync::transaction() and changeVersion() + tests

(In reply to comment #70)
> (From update of attachment 61025 [details])
> I think the db logic looks correct and the new classes are nicely composed. How the SQLTransactionClient class is used is kind of funky. A stateless object but we're making global static instance.
> 
> 51 static SQLTransactionClient* transactionClient()
> 52 {
> 53     DEFINE_STATIC_LOCAL(SQLTransactionClient, transactionClient, ());
> 54     return &transactionClient;
> 55 }
> 
> I don't think DEFINE_STATIC_LOCAL construction macro is thread safe and this getter will definitely be called on different threads. It may be better to just add an SQLTransactionClient data member to SQLTransactionSync and call the methods thru that instance.

done.

[Michael Nordman]

Hi webkit reviewers that happen to be chromium developers too. This patch makes it possible to use sync db api in workers. Would be nice to get it into M6.

[Darin Fisher (:fishd, Google)]

Comment on attachment 61134 [details]
patch #3: implementing DatabaseSync::transaction() and changeVersion() + tests

R=me

[Dumitru Daniliuc]

patch #3 landed: r63278.

[Michael Nordman]

Ooops, it looks like the change actually committed is still setting the version outside of the transaction?


93	void DatabaseSync::changeVersion(const String& oldVersion, const String& newVersion, 
...
125	    if ((ec = transaction->commit()))
126	        return;
127	
128	    setExpectedVersion(newVersion);
129	}


> > Are there any other use cases for the pre/post stuff besides the version change use case? It may be overkill
> > for just this use case. Instead of using runTransaction(), the changeVersion() method could be a slightly 
> > modified form of runTransaction() essentially.
> > 
> > void changeVersion(...) {
> >   trans.begin();
> >   read version from db and compare with expected;
> >   trans.execute(); // invoke the user callback 
> >   write new version to db;
> >   trans.commit();
> > }
> > 
> > wdyt?
> 
> > WebCore/storage/SQLTransactionSync.cpp:208
> >  +      return postflightExceptionCode;
> > Ditto shouldn't we be doing this in the transaction.
> 
> done.
>
> i think doing it atomically is the right way to do it, so in the code i moved the "check version" and "update
> version" steps inside the transaction. i'll send an email to hixie and ask him for a clarification (and we
> should probably make it more clear in the spec too).

[Michael Nordman]

Oh... nevermind :)

120	    if (!setVersionInDatabase(newVersion)) {
121	        ec = SQLException::UNKNOWN_ERR;
122	        return;
123	    }
124	
125	    if ((ec = transaction->commit()))
126	        return;
127	
128	    setExpectedVersion(newVersion);

[Dumitru Daniliuc]

Created attachment 62275 [details]
patch #4: interrupt all DB operations when the worker is shutting down

[Ojan Vafai]

(In reply to comment #78)
> Created an attachment (id=62275) [details]
> patch #4: interrupt all DB operations when the worker is shutting down

Can you move this patch to a separate bug? This could serve as an umbrella bug, but for the sake of committing/reviewing, each bug should have one patch.

[Dumitru Daniliuc]

Comment on attachment 62275 [details]
patch #4: interrupt all DB operations when the worker is shutting down

moving patch #4 to bug 42843, and closing this bug.