Bug 28207 - Another deadlock in the DB code
Summary: Another deadlock in the DB code
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-08-11 18:33 PDT by Dumitru Daniliuc
Modified: 2009-08-18 14:58 PDT (History)
7 users (show)

See Also:


Attachments
patch + layout test (6.94 KB, patch)
2009-08-17 17:53 PDT, Dumitru Daniliuc
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dumitru Daniliuc 2009-08-11 18:33:24 PDT
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.
Comment 1 Michael Nordman 2009-08-13 14:14:04 PDT
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.
Comment 2 Dumitru Daniliuc 2009-08-17 17:53:07 PDT
Created attachment 35004 [details]
patch + layout test
Comment 3 Dimitri Glazkov (Google) 2009-08-18 10:01:30 PDT
Comment on attachment 35004 [details]
patch + layout test

r=me. Good work!
Comment 4 Eric Seidel (no email) 2009-08-18 14:11:51 PDT
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
Comment 5 Eric Seidel (no email) 2009-08-18 14:25:56 PDT
Comment on attachment 35004 [details]
patch + layout test

Sorry, we hit https://bugs.webkit.org/show_bug.cgi?id=28436
Comment 6 Eric Seidel (no email) 2009-08-18 14:58:42 PDT
Comment on attachment 35004 [details]
patch + layout test

Clearing flags on attachment: 35004

Committed r47458: <http://trac.webkit.org/changeset/47458>
Comment 7 Eric Seidel (no email) 2009-08-18 14:58:50 PDT
All reviewed patches have been landed.  Closing bug.