[webkit-reviews] review canceled: [Bug 27966] Race condition in the database code : [Attachment 34233] transaction coordinator + test case

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Aug 6 18:19:03 PDT 2009

Dumitru Daniliuc <dumi at chromium.org> has canceled Dumitru Daniliuc
<dumi at chromium.org>'s request for review:
Bug 27966: Race condition in the database code

Attachment 34233: transaction coordinator + test case

------- Additional Comments from Dumitru Daniliuc <dumi at chromium.org>
(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

> 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.

More information about the webkit-reviews mailing list