Bug 27966 – Race condition in the database code

• Component: New Bugs
• Status: RESOLVED FIXED

[Dumitru Daniliuc]

The WebCore DB code has a bug that can lead to a deadlock. Here are the details:

1. The user creates 2 handles to the same DB on the same page, and executes a transaction on each handle:
    db1 = openDatabase("Foo", "1.0", "Foo", 1000);
    db2 = openDatabase("Foo", "1.0", "Foo", 1000);
    db1.transaction(...);
    db2.transaction(...);

2. The main thread gets to db1.transaction() and schedules a task (#1) on the DB thread that will call SQLTransaction::openTransactionAndPreflight().
3. The main thread gets to db2.transaction() and schedules a task (#2) on the DB thread that will call SQLTransaction::openTransactionAndPreflight().
4. The DB thread executes SQLTransaction::openTransactionAndPreflight() for the first transaction (db1). It calls m_sqliteTransaction->begin() which makes SQLite acquire a lock on the DB file. Then the DB thread calls m_database->scheduleTransactionCallback() (which schedules a task on the main thread) and now the DB thread is done with task #1.
5. The DB thread moves on to task #2, which wants to run db2's transaction. The DB thread tries to execute SQLTransaction::openTransactionAndPreflight() for db2, and when it gets to m_sqliteTransaction->begin(), it blocks, because db1's transaction has the lock on the DB file.
6. So now the DB thread cannot finish transaction #2 until transaction #1 is done and releases the lock on the DB file, and it cannot move on with transaction #1 because it is blocked trying to acquire a lock for transaction #2.

Eventually, the call to m_sqliteDatabase for transaction #2 will fail (when sqlite's busy timeout is reached), transaction #2 will fail, and the DB thread will continue executing transaction #1, which should succeed.


Proposed solution: Add a "transaction coordinator" interface with 2 methods: aquireLock(SQLTransaction*) and releaseLock(SQLTransaction*); and add a step that calls TransactionCoordinator::aquireLock() before calling SQLTransaction::openTransactionAndPreflight() (m_nextStep in SQLTransaction's constructor would be set to this step), and another step that calls TransactionCoordinator::releaseLock() that is called at the end of every transaction.

Here's how the above example would work with these 2 steps:

1. The main thread gets to db1.transaction() and schedules task #1 on the DB thread that will call TransactionCoordinator::aquireLock();
2. The main thread gets to db2.transaction() and schedules task #2 on the DB thread that will call TransactionCoordinator::aquireLock();
3. The DB thread calls aquireLock() for db1's transaction, which returns 'true' (success). The DB thread sets m_nextStep = &SQLTransaction::openTransactionAndPreflight, and calls db1->scheduleTransactionCallback() as it does for all other steps.
4. The DB thread calls aquireLock() for db2's transaction. The transaction coordinator saves transaction #2 to the queue of pending transactions for the given database, and returns 'false'. Since aquireLock() failed, the DB thread does not do anything else related to transaction #2.
5. Eventually, transaction #1 finishes executing (the DB thread is no longer blocked on other transactions), and releaseLock(transaction_1) is called. The transaction coordinator checks the list of pending transactions for the given database, and sees transaction #2 there. Before returning from releaseLock(), the transaction coordinator calls db2->scheduleTransactionCallback() to reschedule transaction #2.
6. Now the DB thread can acquire the lock for transaction #2 and run it without causing any deadlocks.

Thoughts? Suggestions?

[Darin Adler]

My first thought is that given this understanding we must be able to create a test case that demonstrates the deadlock. Can we do this?

[Dumitru Daniliuc]

Created attachment 34075 [details]
test case that demonstrates the problem

(In reply to comment #1)
> My first thought is that given this understanding we must be able to create a
> test case that demonstrates the deadlock. Can we do this?

yes. attached.

[Dumitru Daniliuc]

Created attachment 34233 [details]
transaction coordinator + test case

[Michael Nordman]

I'm not a webkit reviewer... some drive by comments all the same :)

50         OwnPtr<HashMap<String, Deque<SQLTransaction*>*> > m_pendingTransactions;

I think you could put RefPtr<T>'s in the Deque to eliminate the manual refcounting.

51         Mutex m_pendingTransactionsGuard;

I think this should only be called on a particular DB thread, is the Mutex really needed?

47 SQLTransactionCoordinator::~SQLTransactionCoordinator()
48 {
49 }

What happens to transactions that don't actually start prior to the thread being killed. How do they get derefed?

210 void SQLTransaction::lockAquired()
211 {
212     m_nextStep = &SQLTransaction::deliverOpenTransactionAndPreflightCallback;
213     LOG(StorageAPI, "Scheduling deliverOpenTransactionAndPreflight for transaction %p\n", this);
214     m_database->scheduleTransactionCallback(this);
215 }

A the lockAcquired point, the system pings-pongs to the main thread and back again to the db thread. What is the reason for the 'deliverOpenTransactionAndPreflightCallback' step? I'm wondering if we can we skip straight to the openTransactionAndPreflight step at this point, without ping/ponging to the main thread.


217 void SQLTransaction::deliverOpenTransactionAndPreflightCallback()
218 {
219     m_nextStep = &SQLTransaction::openTransactionAndPreflight;
220     LOG(StorageAPI, "Scheduling openTransactionAndPreflight for transaction %p\n", this);
221     m_database->scheduleTransactionStep(this);
222 }

[Dumitru Daniliuc]

Created attachment 34241 [details]
patch + test case

(In reply to comment #4)

good comments!

> 50         OwnPtr<HashMap<String, Deque<SQLTransaction*>*> >
> m_pendingTransactions;
> 
> I think you could put RefPtr<T>'s in the Deque to eliminate the manual
> refcounting.

done. any way to do the same for the Deques? that is, can we wrap them into something that would delete them as soon as we exit the block of code that removes them from the hash map?

> 51         Mutex m_pendingTransactionsGuard;
> 
> I think this should only be called on a particular DB thread, is the Mutex
> really needed?

good catch, removed.

> 47 SQLTransactionCoordinator::~SQLTransactionCoordinator()
> 48 {
> 49 }
> 
> What happens to transactions that don't actually start prior to the thread
> being killed. How do they get derefed?

added code to clean up all pending transactions. i left it empty mostly because i wasn't sure what to do here and was hoping for some comments. is there anything else we might need to do other than cleaning up the transaction queues?

> 210 void SQLTransaction::lockAquired()
> 211 {
> 212     m_nextStep =
> &SQLTransaction::deliverOpenTransactionAndPreflightCallback;
> 213     LOG(StorageAPI, "Scheduling deliverOpenTransactionAndPreflight for
> transaction %p\n", this);
> 214     m_database->scheduleTransactionCallback(this);
> 215 }
> 
> A the lockAcquired point, the system pings-pongs to the main thread and back
> again to the db thread. What is the reason for the
> 'deliverOpenTransactionAndPreflightCallback' step? I'm wondering if we can we
> skip straight to the openTransactionAndPreflight step at this point, without
> ping/ponging to the main thread.

changed lockAquired() to call openTransactionAndPreflight() directly. i scheduled a task on the main thread only because i wanted to release the lock on SQLTransactionCoordinator::m_pendingTransactions as soon as possible (even though i guess it didn't matter, because the DB thread was doing the same amount of work in one order or another). but now that the lock is gone i don't think there's any reason to do this anymore.

[Eric Seidel (no email)]

Comment on attachment 34241 [details]
patch + test case

Please avoid combining style cleanup with functional changes when possible. :)  By the time I got to TransactionCoordinator my eyes were sagging. ;)

[Eric Seidel (no email)]

Comment on attachment 34241 [details]
patch + test case

Style violation:
48         virtual void aquireLock(SQLTransaction* transaction);
 49         virtual void releaseLock(SQLTransaction* transaction);

I've looked again.  This is simply too big for me to review.  You might be able to find one of the Database guys who can do this for you though.  In the future, definitely try to make your patches smaller by not including code-cleanup in functional changes.  I suggest you try to track down one of the folks who have worked on storage via IRC.  Failing that, r- the patch and re-post a smaller piece.  I assume that it's not possible to land the layout test first since it will cause DRT to hang?

[Dumitru Daniliuc]

Sorry, I won't combine clean ups with real changes in the future. I'll see if I can find somebody to review this, and if not, I'll resubmit the patch without the clean up.

The new test will crash without this patch, and storage tests will fail. If this is acceptable and will help with the review, please let me know and I'll put it in a separate patch.

[Dumitru Daniliuc]

Created attachment 34299 [details]
patch + test case

Removed all style changes, should be easier to review now.

[Eric Seidel (no email)]

Comment on attachment 34299 [details]
patch + test case

Thank you!  This is much much more reviewable.

Why do we have one transaction coordinator per thread instead of per database?  if it was per database, then we would not need the hash to hold these queues, we could just hold them on each datbase?  Do transations span across datbases?

I find the test case very hard to read using anonymous functions.  If I were writing that I would have given the functions nice names, even if they were only defined locally.

It seems teh select/insert tests differ only in "db1" vs. "db2".  Probably should be a single funtion which takes a name argument, no?

Likewise the openDatbasse call could be a function to make this more readable.  In general the test has too much copy/paste code. :(

Since no one else has stepped up yet, I think you're stuck with me! ;)

In general this looks great.  The layout test needs some cleanup.  And I need better understanding (ideally with explanation in teh ChagneLog) as to why it's designed this way with teh DatbaseThread holding the transaction coordinator instead of the datbase.  Also confusing that m_database is actually a datbaseThread it seems.

Why is a securityORiginCopy needeD?
 62     return database->securityOriginCopy()->databaseIdentifier() + ":" + database->stringIdentifier();

In general the ChangeLog could be more detailed.

anyway, I look forward to the next round!  This seems like a good fix.  Just needs to be presented more clearly.

[Michael Nordman]

Yes, much more readable without the style cleanup.

51         typedef Deque<RefPtr<SQLTransaction> > TransactionsQueue;
52         typedef HashMap<String, TransactionsQueue*> TransactionsHashMap;
53         OwnPtr<TransactionsHashMap> m_pendingTransactions;

Can you use an OwnPtr<TransactionsQueue> in the HashMap to avoid any manual cleanup in the dtor? When the TransactionsHashMap is deleted i think it would all be properly deleted and deferenced.

Also, can TransactionHashMap be an inline data member instead of held in an OwnPtr?

91     // Remove 'transaction' from the queue of pending transactions
92     // 'transaction' should always be the first transaction in this queue
93     ASSERT(pendingTransactions->first().get() == transaction);
94     pendingTransactions->removeFirst();

Just checking... are there any circumstanaces where a waiting transaction can run thru its 'fail' sequence and call releaseLock() early?

[Dumitru Daniliuc]

(In reply to comment #10)
> (From update of attachment 34299 [details])
> Thank you!  This is much much more reviewable.

Sorry it wasn't like that from the very beginning.

> Why do we have one transaction coordinator per thread instead of per database? 
> if it was per database, then we would not need the hash to hold these queues,
> we could just hold them on each datbase?  Do transations span across datbases?

To answer this, I feel like I need to explain a bit how transactions work in WebCore (sorry if you already know all this). Each transaction in WebCore is a sequence of steps. To start a transaction, the main thread schedules a task on the DB thread that runs the first step. After every step, the DB thread schedules a task back on the main thread that essentially says "OK, I'm done with this step, tell me what to do next". Then the main thread schedules the next step, and so on, until the transaction is complete (successfully, or with an error). So it is very common that the database thread would run a few steps from one transaction, then run a few steps from another transaction, then go back to the first transaction, and so on.

Now to get to your questions. The spec requires openDatabase() to always return a new Database object. WebCore not only returns a new object, but it also returns a new SQLite connection to the database (I believe it's the right thing to do). So you can end up with multiple Database instances (and connections) in the same document that point to the same database file. Now if in your Javascript code you started two transactions on two different Database objects that point to the same physical database (physical database == DB file, in SQLite's case), then it is very probable that the database thread will execute a few steps from your first transaction and then try to run some steps from your second transaction. This would result in a deadlock as described in this bug. So it's not enough to have a transaction queue per Database object (WebCore already has that); what you really want is a way to coordinate transactions across multiple Database instances that point to the same physical DB. This rules out the idea of having a transaction coordinator per Database instance. We could still have a transaction coordinator per "physical database" (that's shared by all Database instances pointing to the same database). However, there's no need to have (# physical databases) transaction coordinators. One instance per database thread is enough to make sure that the DB thread does not deadlock.

> I find the test case very hard to read using anonymous functions.  If I were
> writing that I would have given the functions nice names, even if they were
> only defined locally.
> 
> It seems teh select/insert tests differ only in "db1" vs. "db2".  Probably
> should be a single funtion which takes a name argument, no?
> 
> Likewise the openDatbasse call could be a function to make this more readable. 
> In general the test has too much copy/paste code. :(

refactored it, should be better now.

> Since no one else has stepped up yet, I think you're stuck with me! ;)

oh no... :)

> In general this looks great.  The layout test needs some cleanup.  And I need
> better understanding (ideally with explanation in teh ChagneLog) as to why it's
> designed this way with teh DatbaseThread holding the transaction coordinator
> instead of the datbase.

I added a sentence to ChangeLog to explain the purpose of the transaction coordinator. I'm not sure if it's enough, but at the same time I'm not sure if it's ok to copy-paste my long explanation there either.

> Also confusing that m_database is actually a
> datbaseThread it seems.

Where? I added a DatabaseThread::transactionCoordinator() call, and a Database::transactionCoordinator() call (because SQLTransaction doesn't know about DatabaseThread). So calls like m_database->transactionCoordinator() are perfectly fine: we're asking the Database object on which a transaction is running to give us the transaction coordinator associated with the database thread for this document. Is this what you were referring to?

> Why is a securityORiginCopy needeD?
>  62     return database->securityOriginCopy()->databaseIdentifier() + ":" +
> database->stringIdentifier();

Not needed anymore, fixed. I thought a database thread could service Databases on more than one origin, but I was wrong.

> In general the ChangeLog could be more detailed.

How detailed should I make it?

> anyway, I look forward to the next round!  This seems like a good fix.  Just
> needs to be presented more clearly.

Please take another look.

[Dumitru Daniliuc]

(In reply to comment #11)
> 51         typedef Deque<RefPtr<SQLTransaction> > TransactionsQueue;
> 52         typedef HashMap<String, TransactionsQueue*> TransactionsHashMap;
> 53         OwnPtr<TransactionsHashMap> m_pendingTransactions;
> 
> Can you use an OwnPtr<TransactionsQueue> in the HashMap to avoid any manual
> cleanup in the dtor? When the TransactionsHashMap is deleted i think it would
> all be properly deleted and deferenced.
> 
> Also, can TransactionHashMap be an inline data member instead of held in an
> OwnPtr?

After a long discussion with Michael, we decided to make TransactionsHashMap an inline data member. However, we couldn't find a good way to wrap TransactionsQueues inside the hash map into a smart pointer. Any suggestions here?

> 91     // Remove 'transaction' from the queue of pending transactions
> 92     // 'transaction' should always be the first transaction in this queue
> 93     ASSERT(pendingTransactions->first().get() == transaction);
> 94     pendingTransactions->removeFirst();
> 
> Just checking... are there any circumstanaces where a waiting transaction can
> run thru its 'fail' sequence and call releaseLock() early?

Again, after some investigation with Michael, we found a case where a transaction might fail before it even acquires a lock (Database::stop() is called after scheduling the first step of a transaction on the DB thread, but before the DB thread has a chance to start the task). So we added a m_lockAcquired flag to SQLTransaction.

[Dumitru Daniliuc]

Created attachment 34363 [details]
patch + test case

[Michael Nordman]

> > 51         typedef Deque<RefPtr<SQLTransaction> > TransactionsQueue;
> > 52         typedef HashMap<String, TransactionsQueue*> TransactionsHashMap;
> > 53         OwnPtr<TransactionsHashMap> m_pendingTransactions;
> > 
> > Can you use an OwnPtr<TransactionsQueue> in the HashMap to avoid any manual
> > cleanup in the dtor? When the TransactionsHashMap is deleted i think it would
> > all be properly deleted and deferenced.

I think the mapped type can be the collection class (no ptr indirection involved). The code that operates on the map would have to change to use iterators to access references to the map elements (instead of making copies of the elements via the get(key) method. Then there would be no cleanup code required in the dtor (less code is good). WDYT?

There are examples of HashMap<key, CollectionT> in the code base that you can look at for examples. DOMApplicationCache's EventListenerMap is one example.

> Again, after some investigation with Michael, we found a case where a
> transaction might fail before it even acquires a lock (Database::stop() is
> called after scheduling the first step of a transaction on the DB thread, but
> before the DB thread has a chance to start the task). So we added a
> m_lockAcquired flag to SQLTransaction.

I think the addition of the flag is a nice and easy way to handle the early exit case. We could also use the flag to make some assertions about not having missed other cases provided we reset it upon calling releaseLock() and ASSERT(!m_lockAcquired) in the dtor. We could also ASSERT(m_lockAcquired) in the processing of other transaction steps to make sure we haven't missed acquiring the lock in any cases. WDYT?

[Michael Nordman]

WebKit generally avoids virtual methods where possible (a little less indirection in a lot of places adds up). It doesn't look like there's a need for the methods of the new class to be virtual. (also true for the other patch you have out for review).

In looking at this again, I think there are cases where we'd expect the m_lockAcquired flag to be true in SQLTransaction's dtor. If the DBThread is shutdown (via requestTermination), no further transaction steps will occur since the task queue will have been killed. So an assertion may not be valid. I don't think that presents a problem when this interface is being used to coordinate amoung dbfiles executing on the same dbthread but this may give chromium grief when we're trying to use this interface to coordiate amoungst transactions against the same dbfile system wide.

[Dumitru Daniliuc]

Created attachment 34512 [details]
same patch

Michael, I think I addressed all your comments, please take another look.

m_lockAcquired: I added ASSERTs at the beginning of every task executed on the DB thread (+ openTransactionAndPreflight, which used to be and still kind of is the real first step). I'm also checking this flag in the destructor, and if it's true, I call releaseLock(). Any reason not to do it? It seems to me like this approach should work in Chromium's case too. Minor concern: in order to be able to do this in the destructor, I have to set m_lockAcquired to false everywhere I call releaseLock() (otherwise releaseLock() will be called in the destructor again, and some assertions will fail). Is it somehow possible that releaseLock() is called (and the transaction is taken out of the queue), but m_lockAcquired is not reset to false right after that? I can't think of any case that would result in this behavior.

HashMap of collections: I used the same technique that you used in your code, and cleaned up the destructor. Please double-check that I don't have "refactoring typos" in this code.

[Michael Nordman]

Getting closer...

85 SQLTransaction::~SQLTransaction()
86 {
 87     if (m_lockAcquired) {
 88         m_database->transactionCoordinator()->releaseLock(this);
 89         m_lockAcquired = false;
 90     }
91 }

I'm fairly sure we don't want this call in the SQLTransaction dtor. The object is refcounted and may be finally released on some thread other than the DBThread.

It seems like this case should only come into play after the DBThread is shutdown (as a side effect of the document being closed), otherwise the transaction step processing should have done the right thing by now. I wonder if there may be some refinements to make specifically for the dbthread shutdown. A test case to queue up some xactions and then navigate away from the page could shed some light on things.

This seems clear, SQLTransactionCoordinator should not start new xactions if DatabaseThread.terminationRequested() is true.

Maybe we could use another method on the interface for the thread shutdown case?

    AutodrainedPool pool;
    while (true) {
        RefPtr<DatabaseTask> task;
        if (!m_queue.waitForMessage(task))
            break;

        task->performTask();

        pool.cycle();
    }

    m_transactionCoordiator->shutdown();

The coordinator could take appropiate actions with any remaining items at this time.

[Dumitru Daniliuc]

Created attachment 34535 [details]
patch

Addressed Michael's comments. Also, Michael suggested that it might be better to make SQLTransaction::lockAcquire() schedule an immediate task on the DB thread for SQLTransaction::openTransactionAndPreflight(), instead of directly calling this method. I agreed and made this change too, by adding a bool parameter to Database::scheduleTransactionStep() that defaults to false (true = scheduleImmediateTask(), false = scheduleTask()).

[Michael Nordman]

The code LGTM. Possibly some names could be tweeked to better fit webkit conventions? In all cases, you'll need a webkit reviewer to take a peek.

[Eric Seidel (no email)]

Comment on attachment 34535 [details]
patch

For the future (this one being in pre-existing code), WebKit code tends to favor early returns instead of long if-blocks:
544545     if (m_document->databaseThread()) {

Yes, our style is strange, but comments count as "lines" for multi-line if blocks and shoudl have  { } (I used to get this confused myself):
 87     if (pendingTransactions.isEmpty())
 88         // No more pending transactions; delete dbIdentifier's queue
 89         m_pendingTransactions.remove(it);
 90     else
 91         // We have more pending transactions; start the next one
 92         pendingTransactions.first()->lockAcquired();

No argument names when they don't add clarity:
 46         void acquireLock(SQLTransaction* transaction);
 47         void releaseLock(SQLTransaction* transaction);

Why not add named functions for these:
 34                      // statement success callback
 35                      function(result) { log(dbName + " read statement succeeded"); },
 36 
 37                      // statement failure callback
 38                      function(tx, error) { log(dbName + " read statement failed: " + error.message); });

Then you dont' have to declare them twice, nor do you need the extra lines of comments or spaces because the method names make it obvious what you're doing.

function successCallback(testName) {
return function(result) { log(dbName + " " + testName + " statement succeeded"); }

successCallback("write")

If you wanted, you coudl even make them more general and cover the other places you use success/error callbacks too.

I'm ready to r+ this, but I think we should go one more round (since iirc you are not yet a committer).

[Dumitru Daniliuc]

(In reply to comment #21)
> (From update of attachment 34535 [details])
> For the future (this one being in pre-existing code), WebKit code tends to
> favor early returns instead of long if-blocks:
> 544545     if (m_document->databaseThread()) {

fixed.

> Yes, our style is strange, but comments count as "lines" for multi-line if
> blocks and shoudl have  { } (I used to get this confused myself):
>  87     if (pendingTransactions.isEmpty())
>  88         // No more pending transactions; delete dbIdentifier's queue
>  89         m_pendingTransactions.remove(it);
>  90     else
>  91         // We have more pending transactions; start the next one
>  92         pendingTransactions.first()->lockAcquired();

fixed.

> No argument names when they don't add clarity:
>  46         void acquireLock(SQLTransaction* transaction);
>  47         void releaseLock(SQLTransaction* transaction);

removed.

> Why not add named functions for these:
>  34                      // statement success callback
>  35                      function(result) { log(dbName + " read statement
> succeeded"); },
>  36 
>  37                      // statement failure callback
>  38                      function(tx, error) { log(dbName + " read statement
> failed: " + error.message); });
> 
> Then you dont' have to declare them twice, nor do you need the extra lines of
> comments or spaces because the method names make it obvious what you're doing.
> 
> function successCallback(testName) {
> return function(result) { log(dbName + " " + testName + " statement
> succeeded"); }
> 
> successCallback("write")

done.

> If you wanted, you coudl even make them more general and cover the other places
> you use success/error callbacks too.

we could do this, but since the messages are quite different (read vs. write, statement vs. transaction, and read/write doesn't even apply in case of a transaction callback), i feel like this would only lead to more complicated and unintuitive implementations for successCallback() and errorCallback(), as well as some pretty cryptic calls to these functions.

> I'm ready to r+ this, but I think we should go one more round (since iirc you
> are not yet a committer).

i'm working my way there. :)

[Dumitru Daniliuc]

Created attachment 34701 [details]
patch + test case

[Eric Seidel (no email)]

Comment on attachment 34701 [details]
patch + test case

Passing "read" or "write" instead of a bool would have made the callsites more clear, but this is OK as is.

28 function statementSuccessCallback(dbName, isReadStatement)
 29 {
 30     log(dbName + " " + (isReadStatement ? "read" : "write") + " statement succeeded");
 31 }
 32 
 33 function statementErrorCallback(dbName, isReadStatement, error)

[Dumitru Daniliuc]

Created attachment 34707 [details]
patch + test case

Addressed Eric's last comment.

[Eric Seidel (no email)]

Comment on attachment 34701 [details]
patch + test case

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

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

[Eric Seidel (no email)]

Comment on attachment 34707 [details]
patch + test case

LGTM.

[Eric Seidel (no email)]

Comment on attachment 34707 [details]
patch + test case

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

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

[Eric Seidel (no email)]

Hunk #5 FAILED at 19315.
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 34708 [details]
patch + test case

[Eric Seidel (no email)]

Comment on attachment 34708 [details]
patch + test case

We'll try another round. :)

[Eric Seidel (no email)]

Comment on attachment 34708 [details]
patch + test case

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

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

[Eric Seidel (no email)]

    The following files contain tab characters:

        trunk/WebCore/ChangeLog

    Please use spaces instead to indent.

[Eric Seidel (no email)]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	LayoutTests/ChangeLog
	A	LayoutTests/storage/multiple-transactions-on-different-handles-expected.txt
	A	LayoutTests/storage/multiple-transactions-on-different-handles.html
	M	WebCore/ChangeLog
	M	WebCore/GNUmakefile.am
	M	WebCore/WebCore.gypi
	M	WebCore/WebCore.vcproj/WebCore.vcproj
	M	WebCore/WebCore.xcodeproj/project.pbxproj
	M	WebCore/storage/Database.cpp
	M	WebCore/storage/Database.h
	M	WebCore/storage/DatabaseThread.cpp
	M	WebCore/storage/DatabaseThread.h
	M	WebCore/storage/SQLTransaction.cpp
	M	WebCore/storage/SQLTransaction.h
	A	WebCore/storage/SQLTransactionCoordinator.cpp
	A	WebCore/storage/SQLTransactionCoordinator.h
Committed r47170
	M	WebCore/storage/DatabaseThread.h
	M	WebCore/storage/SQLTransaction.cpp
	M	WebCore/storage/Database.h
	A	WebCore/storage/SQLTransactionCoordinator.cpp
	M	WebCore/storage/DatabaseThread.cpp
	M	WebCore/storage/Database.cpp
	M	WebCore/storage/SQLTransaction.h
	A	WebCore/storage/SQLTransactionCoordinator.h
	M	WebCore/ChangeLog
	M	WebCore/WebCore.vcproj/WebCore.vcproj
	M	WebCore/GNUmakefile.am
	M	WebCore/WebCore.gypi
	M	WebCore/WebCore.xcodeproj/project.pbxproj
	M	LayoutTests/ChangeLog
	A	LayoutTests/storage/multiple-transactions-on-different-handles-expected.txt
	A	LayoutTests/storage/multiple-transactions-on-different-handles.html
r47170 = 02db11b4350373f83f85041c33de18fb7410ec8b (trunk)
No changes between current HEAD and refs/remotes/trunk
Resetting to the latest refs/remotes/trunk
http://trac.webkit.org/changeset/47170

[Eric Seidel (no email)]

Fixed and landed manually.