Bug 28207 – Another deadlock in the DB code

• Component: New Bugs
• Status: RESOLVED FIXED

[Dumitru Daniliuc]

Bug 27966 describes how 2 transactions run on 2 different handles to the same DB can deadlock the DB thread. Here's an example of how a transaction combined with a openDatabase() call can deadlock the DB thread and block the main thread at the same time:

1: var db1 = openDatabase("SomeDB", "1.0", "Blah", 10000);
2: db1.transaction(function(tx) {
3:     tx.executeSql("SomeWriteStatement", [args],
4:         function(result) { var db2 = openDatabase("SomeDB", "1.0", "Blah", 10000); },
5:         function(tx, error) { });
6:     },
7:     function(error) { },
8:     function() { }
9: );

Description:
1. The user opens a handle to a database (line #1).
2. The user starts running a transaction on that handle (line #2). The transaction must have a "write" statement, that will make SQLite acquire an exclusive lock on the DB file (line #3).
3. Eventually, we get to running the statement callback (line #4).
4. openDatabase() is called on the main thread (line #4).
5. The main thread creates a new SQLite connection to the same DB file, schedules a task on the DB thread (to make sure the version is correct), and blocks waiting for this task to complete (openDatabase() is a sync call according to the spec).
6. In order to check the version, the new task needs to acquire a "read" lock on the DB file first and check if the __WebKitDatabaseInfoTable__ table exists.
7. This "read" lock cannot be acquired because the transaction has an exclusive lock on the DB file. At the same time, the transaction cannot release the exclusive lock until the statement callback is executed. So the DB thread is deadlocked, and the main thread is blocked too, because it waits for the openDatabase() call to finish.

Safari's debug build from trunk deadlocks for 30 seconds (SQLite's busy_timeout) and then crashes.


PROPOSED FIX:

It's OK for the first call to openDatabase(SomeDB, ...) to execute SQL statements, because we know there are no in-flight transactions on other DB handles to the same database. However, the 2nd, 3rd, ... calls to openDatabase(SomeDB, ...) cannot run any SQL statements without risking a deadlock. So in addition to caching the DB version (this is what WebCore does now), we need to also cache a flag that tells us if the __WebKitDatabaseInfoTable__ exists in the database.

[Michael Nordman]

Good bug.

Agreed that the root cause of this problem is the synchronous window.openDatabase method that must read the 'version' attribute value, and that caching that value and using the cached value at open time should help.

I don't follow how caching whether the __WebKitDatabaseInfoTable__ exists solves the problem. (i see what your driving at now, because it gets created prior to checking for the cached version, we could just move the table creation till a little further along in all cases).

Seems like this could be fixed within performOpenAndVerify().

If a cached version value exists, don't bother touching the dbfile in anyway beyond opening the sqlite3 handle, simply trust the cached value.

Otherwise its the first opening of this DB in this session. (Create the special table if needed and) read the version value from the DB and populate the cache with that value.

Provided this happens behind the guidMutex, i think there won't be a deadlock.

[Dumitru Daniliuc]

Created attachment 35004 [details]
patch + layout test

[Dimitri Glazkov (Google)]

Comment on attachment 35004 [details]
patch + layout test

r=me. Good work!

[Eric Seidel (no email)]

Comment on attachment 35004 [details]
patch + layout test

Rejecting patch 35004 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 35004 [details]
patch + layout test

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

[Eric Seidel (no email)]

Comment on attachment 35004 [details]
patch + layout test

Clearing flags on attachment: 35004

Committed r47458: <http://trac.webkit.org/changeset/47458>

[Eric Seidel (no email)]

All reviewed patches have been landed.  Closing bug.