[Webkit-unassigned] [Bug 27966] Race condition in the database code

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=27966





--- Comment #12 from Dumitru Daniliuc <dumi at chromium.org>  2009-08-07 22:34:47 PDT ---
(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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.