Bug 25711 – HTML5 Database becomes locked if a transaction is in progress when the page is refreshed.

• Component: New Bugs
• Status: RESOLVED FIXED

[Andrew Grieve]

Happens with both read and write transactions. See attached test harness for reproducing the bug.

[Andrew Grieve]

Created attachment 30212 [details]
Test harness for reproducing the bug.

[Ben Murdoch]

I have a fix for this bug here: https://bugs.webkit.org/show_bug.cgi?id=25788

[David Kilzer (:ddkilzer)]

<rdar://problem/6860537>

[Ben Murdoch]

Created attachment 30483 [details]
Proposed fix and layout test

Copied this fix across from duplicate bug 25788. I will close the duplicate.

[Ben Murdoch]

*** Bug 25788 has been marked as a duplicate of this bug. ***

[Darin Adler]

Comment on attachment 30483 [details]
Proposed fix and layout test

Seems like the sleep should be shared/abstracted in WTF.

[Eric Seidel (no email)]

Comment on attachment 30483 [details]
Proposed fix and layout test

Mostly style violations:
http://webkit.org/coding/coding-style.html

 622 int Database::busyHandler(void* p, int n) {
bad argument names, { goes on a new line

no { } needed:
4     while (n--) {
 625         millis += 1.2*millis;
 626     }
although we've been inconsistend w/ whiles.

Also spacing between 1.2 and millis (Which should be a full name, like milliseconds)

We use constants instead of defines where possible:
 55 #define MAX_SQLITE_BUSY_WAIT_TIME 30000

Variable names:
 122     static int busyHandler(void* p, int n);

Tabs in the layout tests.

Otherwise this looks fine in general, but really you shoudl have Bradee look at it for content once style is fixed.

[Ben Murdoch]

(In reply to comment #6)
> Seems like the sleep should be shared/abstracted in WTF.

Agreed, that sounds like a good idea. I was actually looking in WTF for a sleep function which is how I found the code in TCSpinLock :) Shall I add a new file pair (say Sleep.h|cpp) or is there a better place in  WTF for them to go already?

(In reply to comment #7)
> (From update of attachment 30483 [details] [review])
> Mostly style violations:
> http://webkit.org/coding/coding-style.html
> 

Thanks for the style points Eric, I will fix them up and check out the style guide.

[Darin Adler]

(In reply to comment #8)
> Agreed, that sounds like a good idea. I was actually looking in WTF for a sleep
> function which is how I found the code in TCSpinLock :) Shall I add a new file
> pair (say Sleep.h|cpp) or is there a better place in  WTF for them to go
> already?

I think they could go with the threading functions since they're used together.

[Ben Murdoch]

Created attachment 30672 [details]
Corrects style and moves the sleep into WTF.

[Michael Nordman]

> +    m_sqliteDatabase.setBusyHandler(&Database::busyHandler);
...
> +int Database::busyHandler(void* context, int consecutiveInvocations)
> +{
> +    UNUSED_PARAM(context);
> +
> +    int milliseconds = 100;
> +    while (consecutiveInvocations--)
> +        milliseconds += 1.2F * milliseconds;
> +	WTF::sleepMilliseconds(milliseconds);
> +    return milliseconds >= MAX_SQLITE_BUSY_WAIT_TIME ? 0 : 1;
> +}


Do you actually need a custom busyHandler? The default busy handler in sqlite has reasonable backoff logic built in (see sqliteDefaultBusyCallback). So I wonder if you can accomplish the same thing with less code in webcore... just database.setBusyTimeout(30seconds).

Also, about the backoff logic in your handler... I think its not going to do what you want.

It looks like it will not return 0 until the handler sleeps at least 30 seconds in one go. The total time prior to a sqlite function failing with a SQLITE_BUSY error is going to be considerably longer than 30 seconds since sqlite will be calling it repeatedly until its either no longer busy or the handler returns 0;

Also, theres extra whitespace in front of WTF::sleepMilliseconds

[Ben Murdoch]

Hi Michael,

> 
> Do you actually need a custom busyHandler? The default busy handler in sqlite
> has reasonable backoff logic built in (see sqliteDefaultBusyCallback). So I
> wonder if you can accomplish the same thing with less code in webcore... just
> database.setBusyTimeout(30seconds).

Ah, I see. I wasn't aware of sqliteDefaultBusyCallback and that that was how the setBusyTimeout function was implemented. It seems to work fine, so I'll send a new patch to use it instead.

Thanks for the tip!

[Ben Murdoch]

Created attachment 30747 [details]
New patch and layout test

Uses the default busy timeout rather than a custom busy handler.

[Michael Nordman]

Less is more ;)

> file: Database.cpp
> #include <wtf/Threading.h>

You may not need this addition anymore.

[Ben Murdoch]

Created attachment 30751 [details]
New patch.

Thanks Michael :)

Also fixed a typo in the Changelog entry.

[Ben Murdoch]

Created attachment 30902 [details]
Makes the layout test pass on Windows.

Stops calling an unnecessary (and unimplemented on windows) layoutTestController method.

[Ben Murdoch]

Ping...

[Michael Nordman]

I don't have review rights... but fwiw...

File: WebCore/storage/SQLTransaction.cpp

nit: A comment in handleTransactionError could read differently
// No error callback, so fast-forward to:
// No error callback or the database is stopping, so fast-forward to:
Actually, you may want to remove the comment since it just says in english what the branching logic is clearly doing... doesn't add value really.

If inCallback && m_database->stopped(), can you bump into the same problem with scheduling the next cleanupAfterTransactionErrorCallback step? If that combo can't happen, maybe add an ASSERT. Or if that combo can happen, maybe just if (stopped) {cleanupAfter(); return;} at the top of the method.

[Ben Murdoch]

(In reply to comment #18)

Thanks again for the comments Michael.

> If inCallback && m_database->stopped(), can you bump into the same problem with
> scheduling the next cleanupAfterTransactionErrorCallback step? If that combo
> can't happen, maybe add an ASSERT. Or if that combo can happen, maybe just if
> (stopped) {cleanupAfter(); return;} at the top of the method.

Good catch! This got me thinking and I believe that we need to protect each point that cleanup steps may be executed from the database having been stopped. The main thread could stop the database at any time, therefore it's imperative that after a statement is executed we ensure that the commit or rollback steps can be completed synchronously if the database has been stopped. I've checked that each path of execution after m_currentStatement->execute in SQLTransaction::runCurrentStatement can do this and I think it's now OK.

Sending a new patch.

[Ben Murdoch]

Created attachment 31133 [details]
New patch.

See comment above.

[Michael Nordman]

I think you may have some race conditions?

Database::stop() can get called at any time while the db thread is working. So in the following...

 506         if (m_database->stopped()) {
 507             LOG(StorageAPI, "Database stopped, executing cleanupAfterTransactionErrorCallback for transaction %p\n", this);
 508             cleanupAfterTransactionErrorCallback();
 509         } else {
 510             m_nextStep = &SQLTransaction::cleanupAfterTransactionErrorCallback;
 511             LOG(StorageAPI, "Scheduling cleanupAfterTransactionErrorCallback for transaction %p\n", this);
 512             m_database->scheduleTransactionStep(this);
 513         }

... stopped() can return false on line 506, but by the time we get to line 512 it would return true


Maybe instead scheduleTransactionStep() could return a boolean value indicating if the step was scheduled or not due to stop having been called. And should interlock with the stop() method. If false is returned, the db thread would run the cleanup code.

void Database::scheduleTransactionStep(SQLTransaction* transaction)
{
    MutexLocker locker(m_stopMutex);
    if (stopped())
      return false;
    if (m_document->databaseThread()) {
        RefPtr<DatabaseTransactionTask> task = DatabaseTransactionTask::create(transaction);
        LOG(StorageAPI, "Scheduling DatabaseTransactionTask %p for the transaction step\n", task.get());
        m_document->databaseThread()->scheduleTask(task.release());
    }
    return true;
}

[Ben Murdoch]

Thanks again for the comments Michael. You're right, that would be a race condition. It's been a while since I looked at the root of this bug/patch so I decided to go back and try a different approach... I think the new solution is much cleaner. The SQLite documentation mentions that if you call sqlite3_close on a database with an open transaction, that transaction is automatically rolled back. The call to close needs to come from the database thread so that we can be sure a statement isn't executing when the call is made. So, my new approach is to maintain a list in the DatabaseThread class of the databases that we have worked on in that thread. Before that thread terminates, I iterate that list and invoke close on each database. So if a transaction had scheduled cleanup steps on the thread asynchronously it doesn't matter as the transaction will  be rolled back and the database unlocked.

New patch to follow, any comments appreciated.

[Ben Murdoch]

Created attachment 31364 [details]
New patch.

New patch as above.

[Michael Nordman]

A more systemic approach... nice!

In general I like it. "If the thread is shutting down, be sure to close any handles the thread has ever worked with." More systemically addresses the cause of the bug you're bumping into.

I think this approach is valid since tasks for a given Database are always handled on the same DatabaseThread (looks like theres a db thread per document). If that were ever to change, this approach may not be valid. Not sure what the Database 'owners' have in mind for future developments.

Have you considered removing the the handle from the collection when a DatabaseCloseTask executes on the thread? The expected case is that none in the collection should need to be closed. May be nice of that corresponded with an empty collection. A method on Database could be useful for determining when the Database needs to be added/removed from the set... bool Database.isOpen(). After task execution, if isOpen() add it, otherwise remove it.

I think you can use HashSet<RefPtr<Database> > to avoid the manual addref/removeref calls. And after iterating and calling close on them, clear the collection to drop the refs.

[Ben Murdoch]

(In reply to comment #24)

Thanks again Michael. :)

> I think this approach is valid since tasks for a given Database are always
> handled on the same DatabaseThread (looks like theres a db thread per
> document).

That's right.

> If that were ever to change, this approach may not be valid. Not
> sure what the Database 'owners' have in mind for future developments.

Any webkit reviewers care to comment?

> Have you considered removing the the handle from the collection when a
> DatabaseCloseTask executes on the thread? The expected case is that none in the
> collection should need to be closed. May be nice of that corresponded with an
> empty collection. A method on Database could be useful for determining when the
> Database needs to be added/removed from the set... bool Database.isOpen().
> After task execution, if isOpen() add it, otherwise remove it.

Done.

> I think you can use HashSet<RefPtr<Database> > to avoid the manual
> addref/removeref calls. And after iterating and calling close on them, clear
> the collection to drop the refs.

Done.

New patch on the way! :)

[Ben Murdoch]

Created attachment 31408 [details]
New patch.

[Antti Koivisto]

Comment on attachment 31408 [details]
New patch.

It seems unfortunate that we have to add another database tracking mechanism. There is already the DatabaseTracker and per Document set of databases. The Document set seems to hold similar documents as the new per-db-thread set. 

> +        Database* database = task->database();
> +        if (database->opened()) 
> +            recordDatabaseOpen(database);
> +        else
> +            recordDatabaseClosed(database);
> +

This seems unoptimal. The task loop is not really the right place. Could you remove this code, protect the m_databases set with a mutex and record the databases directly from Database ctor/dtor? Then this

> +    DatabaseSet::iterator end = m_databases.end();
> +    for (DatabaseSet::iterator it = m_databases.begin(); it != end; ++it)
> +        (*it)->close();

could just check for closed() state before invoking close() (or it could be done in close() itself)

Minor nits:

>      void close();
> +    bool opened() const { return m_open; }

Names should match. Rename m_open to m_opened.

> +    // If we sleep for more the 30 seconds while blocked on SQLITE_BUSY, give up.
> +    static const int MAX_SQLITE_BUSY_WAIT_TIME = 30000;

WebKit style is to use camel case for constants too. This does not seem to need to be in header, top of the .cpp would be better.

r- for now, I think at least the recordDatabaseOpen/recordDatabaseClosed in task loop should be cleaned up.

[Ben Murdoch]

Created attachment 31934 [details]
New patch based on Antti's comments.

[Michael Nordman]

Is the Mutex really needed? Can it be replaced with ASSERT(isDatabaseThread())? I think the db should always be opened/close on the db thread, that looks like the intent of things, why not assert it.

The code right now looks like it depends on being able to reenter the mutex since the loop at thread end locks it and the db.close method will also grab the lock. Is WTF::Mutex reentrant on all platforms?

[Ben Murdoch]

(In reply to comment #29)
> Is the Mutex really needed? Can it be replaced with ASSERT(isDatabaseThread())?
> I think the db should always be opened/close on the db thread, that looks like
> the intent of things, why not assert it.
> 
> The code right now looks like it depends on being able to reenter the mutex
> since the loop at thread end locks it and the db.close method will also grab
> the lock. Is WTF::Mutex reentrant on all platforms?
> 

Thanks for the review Michael. It seems that the platform mutexes are not guaranteed to be recursive so this strategy isn't going to work. After some discussion I've decided to move the code for tracking the databases currently opened on a particular thread into the DatabaseTracker (which seems an appropriate place :)). The tracker now has a hash map of thread -> DatabaseSet and whenever a database is added/removed from the tracker I update this set too. I added a new function on the tracker that iterates this map for a given database thread and executes the close. I call that function just before the database thread exits.

Patch is on the way!

[Michael Nordman]

Hmmmm... I liked how the collection was a private detail of DatabaseThread. The "don't forget to close what you opened" behavior was well encapsulated in that class, no other agency was involved. Its been gradually moving away from that with greater fine grained dependencies of other database classes just a ptr deref away from each other, first more into the Database class, and now into the DatabaseTracker class.

The reason I mention this is that we have plans to "split" the DatabaseTracker class for use in chrome's multi-process, sandboxed world. This function (maintaining a collection of what database is opened on what thread and then acting on that at thread end) would be one more hair to split along those lines.

Removing the test after execution of each DatabaseTask was an improvement. The only real issue with that improvement was the Mutex usage (a smaller issue was copying the collection on exit). I think open/close is always be called on the database thread directly. If thats true, please consider simply removing the Mutex.

[Ben Murdoch]

Created attachment 32125 [details]
New patch. (please do not land this version)

[Ben Murdoch]

(In reply to comment #31)
> Hmmmm... I liked how the collection was a private detail of DatabaseThread. The
> "don't forget to close what you opened" behavior was well encapsulated in that
> class, no other agency was involved. Its been gradually moving away from that
> with greater fine grained dependencies of other database classes just a ptr
> deref away from each other, first more into the Database class, and now into
> the DatabaseTracker class.
> 
> The reason I mention this is that we have plans to "split" the DatabaseTracker
> class for use in chrome's multi-process, sandboxed world. This function
> (maintaining a collection of what database is opened on what thread and then
> acting on that at thread end) would be one more hair to split along those
> lines.
> 
> Removing the test after execution of each DatabaseTask was an improvement. The
> only real issue with that improvement was the Mutex usage (a smaller issue was
> copying the collection on exit). I think open/close is always be called on the
> database thread directly. If thats true, please consider simply removing the
> Mutex.
> 

OK, if leaving it in the DatabaseThread makes the coming restructuring for Chrome easier, I'm fine with that. I'm not sure there's a better way to iterate the set other than taking a copy as we call close on each value in the set, which then removes it from the set and hence modifies what we're iterating over. I guess we could instead construct a vector of databases to close and then loop over it to actually call close... does rather seem like six of one and half a dozen of the other though :). I've removed the mutex and replaced it with a couple of asserts.

Cheers, Ben

[Antti Koivisto]

Comment on attachment 32125 [details]
New patch. (please do not land this version)

Is it really always true that recordDatabaseOpen/recordDatabaseClose can't get called in the database thread? If so then obviously you won't need mutexes, but please make sure.

> +    // Close the databases that we ran transactions on. This ensures that if any transactions are still open, they are rolled back and we don't leave the database in an
> +    // inconsistent or locked state.
> +    if (m_openDatabaseSet.size() > 0) {
> +        // As the call to close will modify the original set, we must take a copy to iterate over.
> +        DatabaseSet openSetCopy = m_openDatabaseSet;
> +        DatabaseSet::iterator end = openSetCopy.end();
> +        for (DatabaseSet::iterator it = openSetCopy.begin(); it != end; ++it)
> +           (*it)->close();

You should use HashSet::swap() instead of making a copy before iterating.

r=me with swap()

[Ben Murdoch]

Hi Antti!

Thanks for the review, glad that we're very almost done!

> Is it really always true that recordDatabaseOpen/recordDatabaseClose can't get
> called in the database thread? If so then obviously you won't need mutexes, but
> please make sure.

Yes, at the moment the only times recoredDatabaseOpen/Close are called are in DatabaseOpen/Close tasks that run on the database thread, or in the code I added to close the databases before the thread terminates (naturally on the database thread).

> > +    // Close the databases that we ran transactions on. This ensures that if any transactions are still open, they are rolled back and we don't leave the database in an
> > +    // inconsistent or locked state.
> > +    if (m_openDatabaseSet.size() > 0) {
> > +        // As the call to close will modify the original set, we must take a copy to iterate over.
> > +        DatabaseSet openSetCopy = m_openDatabaseSet;
> > +        DatabaseSet::iterator end = openSetCopy.end();
> > +        for (DatabaseSet::iterator it = openSetCopy.begin(); it != end; ++it)
> > +           (*it)->close();
> 
> You should use HashSet::swap() instead of making a copy before iterating.

OK, but if I do that then in the case where a database is closed by this code, the ASSERT(m_openDatabaseSet.contains(database)) in recordDatabaseClosed() called by Database::close() will fail as we will have swapped the set with an empty set. What is preferable? Keep as is and do the copy and ASSERT in recordDatabaseClosed (I don't think this is often going to be a very large copy) or use swap instead of copy and remove the assert? In my opinion keeping the assert is nicer. What do you think?

Thanks, Ben

[Antti Koivisto]

(In reply to comment #35)
> Hi Antti!
> 
> Thanks for the review, glad that we're very almost done!
> 
> > Is it really always true that recordDatabaseOpen/recordDatabaseClose can't get
> > called in the database thread? If so then obviously you won't need mutexes, but
> > please make sure.
> 
> Yes, at the moment the only times recoredDatabaseOpen/Close are called are in
> DatabaseOpen/Close tasks that run on the database thread, or in the code I
> added to close the databases before the thread terminates (naturally on the
> database thread).
> 
> > > +    // Close the databases that we ran transactions on. This ensures that if any transactions are still open, they are rolled back and we don't leave the database in an
> > > +    // inconsistent or locked state.
> > > +    if (m_openDatabaseSet.size() > 0) {
> > > +        // As the call to close will modify the original set, we must take a copy to iterate over.
> > > +        DatabaseSet openSetCopy = m_openDatabaseSet;
> > > +        DatabaseSet::iterator end = openSetCopy.end();
> > > +        for (DatabaseSet::iterator it = openSetCopy.begin(); it != end; ++it)
> > > +           (*it)->close();
> > 
> > You should use HashSet::swap() instead of making a copy before iterating.
> 
> OK, but if I do that then in the case where a database is closed by this code,
> the ASSERT(m_openDatabaseSet.contains(database)) in recordDatabaseClosed()
> called by Database::close() will fail as we will have swapped the set with an
> empty set. What is preferable? Keep as is and do the copy and ASSERT in
> recordDatabaseClosed (I don't think this is often going to be a very large
> copy) or use swap instead of copy and remove the assert? In my opinion keeping
> the assert is nicer. What do you think?

I think you could just change the assert: ASSERT(m_queue.killed() || m_openDatabaseSet.contains(database)).

Asserts as they are won't catch anything interesting during termination (you obviously are removing only databases that are in the set since you are iterating the set). 


> Thanks, Ben
> 

[Eric Seidel (no email)]

I'll leave this to someone else to land since there are modifications left for the patch.

[Ben Murdoch]

Created attachment 32235 [details]
Patch addressing Antti's final comments. (please submit this version of the patch)

Implements swap instead of a copy and changes the assert as per Antti's comments. As it's a small change to an r+'d patch I'm not marking the old patch obsolete so that this remains in the queue of bugs with patches ready to submit, hope that is OK.

Cheers, Ben

[Jan Alonzo]

(In reply to comment #38)
> Created an attachment (id=32235) [details]
> Patch addressing Antti's final comments. (please submit this version of the
> patch)
> 
> Implements swap instead of a copy and changes the assert as per Antti's
> comments. As it's a small change to an r+'d patch I'm not marking the old patch
> obsolete so that this remains in the queue of bugs with patches ready to
> submit, hope that is OK.
> 
> Cheers, Ben

Landed in r45594. I fixed two style issues with the braces below before landing. 

> +void DatabaseThread::recordDatabaseOpen(Database* database) {
> +    ASSERT(currentThread() == m_threadID);
> +    ASSERT(database);
> +    ASSERT(!m_openDatabaseSet.contains(database));
> +    m_openDatabaseSet.add(database);
> +}
> +
> +void DatabaseThread::recordDatabaseClosed(Database* database) {
> +    ASSERT(currentThread() == m_threadID);
> +    ASSERT(database);
> +    ASSERT(m_queue.killed() || m_openDatabaseSet.contains(database));
> +    m_openDatabaseSet.remove(database);
> +}
> +
>  void DatabaseThread::scheduleTask(PassRefPtr<DatabaseTask> task)