[oilpan]: RFC: Change DatabaseBackendBase::performOpenAndVerify locking

[oilpan]: RFC: Change DatabaseBackendBase::performOpenAndVerify locking

This is a request for comment and should not necessarily be commited.

When looking at the timeout in the terminate-during-sync-operation-websql.html test I found that we are holding a global lock (guidMutex) while doing sync database operations.
The tests starts multiple threads all of which try to open the database. One thread gets the lock and the remaining will block waiting for the lock. None of these threads are in a safepoint at this point. I cannot add a safepoint scope at this point since the code will do allocations.
Instead what I have tried is to release the lock while doing the database operations. This does mean multiple threads might end up trying to create the info table, but I have changed the code so only one of them get to commit the transaction to be as close to the previous semantic as possible.

I should mention that all the waiting threads might not be in performOpenAndVerify, but common is that they are all waiting on the guidMutex which is taken in other methods. Basically any thread taking the guidMutex can block other threads progress since it is a global lock. Doing a sync operation while holding this lock is as such not a good idea.

This change does make the test work without timing out.

R=ager@chromium.org, erik.corry@gmail.com, haraken@chromium.org, oilpan-reviews@chromium.org, tkent@chromium.org, vegorov@chromium.org, zerny@chromium.org, kinuko@chromium.org, morrita@chromium.org
BUG=375671

[Vyacheslav Egorov (Google), 2014-06-13 12:49:40]

https://codereview.chromium.org/335823002/diff/1/Source/modules/webdatabase/D...
File Source/modules/webdatabase/DatabaseBackendBase.cpp (left):

https://codereview.chromium.org/335823002/diff/1/Source/modules/webdatabase/D...
Source/modules/webdatabase/DatabaseBackendBase.cpp:332: MutexLocker
locker(guidMutex());
An alternative less intrusive approach would be to have SafePoint around attempt
to lock this mutex, similar to how we handle thread detachment. 

ThreadState::current()->enterSafePoint(...);
MutexLocker locker(...)
ThreadState::current()->leaveSafePoint();

For the best effect this can be wrapped into the SafePointAwareMutexLocker
class.

We can even have SafePointAwareMutex which would require
SafePointAwareMutexLocker to be used on it and make all database related mutexes
SafePointAware.

[wibling-chromium, 2014-06-13 13:03:12]

On 2014/06/13 12:49:40, Vyacheslav Egorov (Google) wrote:
>
https://codereview.chromium.org/335823002/diff/1/Source/modules/webdatabase/D...
> File Source/modules/webdatabase/DatabaseBackendBase.cpp (left):
> 
>
https://codereview.chromium.org/335823002/diff/1/Source/modules/webdatabase/D...
> Source/modules/webdatabase/DatabaseBackendBase.cpp:332: MutexLocker
> locker(guidMutex());
> An alternative less intrusive approach would be to have SafePoint around
attempt
> to lock this mutex, similar to how we handle thread detachment. 
> 
> ThreadState::current()->enterSafePoint(...);
> MutexLocker locker(...)
> ThreadState::current()->leaveSafePoint();
> 
> For the best effect this can be wrapped into the SafePointAwareMutexLocker
> class.
> 
> We can even have SafePointAwareMutex which would require
> SafePointAwareMutexLocker to be used on it and make all database related
mutexes
> SafePointAware.

Good idea. I will try to create a SafePointAwareMutex and use that instead. That
should fix our GC safepoint issue. However it might still be worth to refactor
the db code to not hold a global lock while doing sync operations, but we can
leave that for a separate fix.

[haraken, 2014-06-13 14:37:14]

Slava's idea sounds great to me. I also agree with Gustav that we should
refactor the webdatabase code to not hold a global lock while doing sync
operations.

[tkent, 2014-06-16 00:03:26]

wibling@, thank you for the investigation and proposing a fix!

I agree with the SafePointAwareMutex idea.  IMO, improving Web Database code is
not worth to do because it's a dying feature.