Bug 204379 – IndexedDB: lock-based implementation of suspension

• Component: New Bugs
• Status: RESOLVED FIXED

[Sihui Liu]

When process is about to suspend, we would:
1. suspend the database thread (by making it wait on a condition variable)
2. (wait on main thread until 1 finishes) stop all transactions on the main thread
To implement this, database thread would check whether it needs to suspend *after each task*. If it needs to, it would wake up the main thread, and wait on suspend condition.

However, if the database thread is already suspended (e.g. waiting on lock) in the middle of some task, the thread will not get to the point of checking suspension state and waking up main thread. Main thread would be stuck.

[Sihui Liu]

Created attachment 383907 [details]
Patch

[Brady Eidson]

Comment on attachment 383907 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=383907&action=review

> Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabase.h:273
> +    Lock m_backingStoreLock;

This change adds fragility.

I think it would make it easier to avoid future mistakes if the backing store itself had a notion of a lock that it could assert is locked for each backing store operation.

I can think of at least two ways to accomplish this:

1 - Since UniqueIDBDatabase needs a lock for the setting/getting of the backing store itself, add a second lock to the backing store.
2 - Pass a Lock& to the backing store constructor so it can ASSERT the lock is appropriately held.

[Brady Eidson]

Comment on attachment 383907 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=383907&action=review

> Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabase.cpp:2473
> +    if (m_isSuspended)
> +        return;
> +    m_isSuspended = true;
> +
> +    m_backingStoreLock.lock();
> +

This change adds fragility.

To make it harder to introduce future mistakes I think we should make this be ASSERT-crazy.

If not suspended, assert the lock is not held.

> Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabase.cpp:2508
> +void UniqueIDBDatabase::resume()
> +{
> +    ASSERT(isMainThread());
> +
> +    if (!m_isSuspended)
> +        return;
> +
> +    m_isSuspended = false;
> +    m_backingStoreLock.unlock();
> +}

This change adds fragility.

To make it harder to introduce future mistakes I think we should make this be ASSERT-crazy.

If suspended, assert the lock is held

[Sihui Liu]

Created attachment 384006 [details]
Patch

[Sihui Liu]

Created attachment 384007 [details]
Patch

[Sihui Liu]

Created attachment 384059 [details]
Patch

[Geoffrey Garen]

A design pattern that can help you remember to take the lock is to require a lock holder reference as an argument to each function that wants the lock to be held. 

That way, the compiler will verify that you did acquire the lock before calling the function. (Technically, you can still make a mistake and lock a different lock of the same type, but that mistake is very rare.)

If we do it that way, we don't really need the backing store to hold the lock as a data member or assert that it is locked; the type of the locker can only be instantiated by locking the lock.

[Geoffrey Garen]

It would also be nice to add a comment next to the declaration of the backing store lock that explains that its purpose is to synchronize against process suspension (even though the backing store is usually only accessed on one thread).

Maybe we could even rename it to backingStoreSuspensionLock.

[Geoffrey Garen]

Here's an alternative solution to consider: What if, before calling the quota manager on the main thread, the database thread pre-emptively suspended its own task queue?

Like so (similar to CrossThreadTaskHandler::taskRunLoop()):

            m_suspendedLock.lock();
            ASSERT(!m_suspended);
            m_suspended = true;
            m_suspendedCondition.notifyOne();
            m_suspendedLock.unlock();

And then, upon reply, it checked whether to suspend?

Like so (duplicating CrossThreadTaskHandler::taskRunLoop()):

        Locker<Lock> shouldSuspendLocker(m_shouldSuspendLock);
        while (m_shouldSuspend) {
            m_suspendedLock.lock();
            if (!m_suspended) {
                m_suspended = true;
                m_suspendedCondition.notifyOne();
            }
            m_suspendedLock.unlock();
            m_shouldSuspendCondition.wait(m_shouldSuspendLock);
        }

I believe, if you did that, you would solve the conundrum of waiting for acknowledgement from the database thread because the database thread would acknowledge being suspended preemptively before it went to sleep; and it would honor any request to suspend after it woke up.

(Of course, rather than duplicating code from CrossThreadTaskHandler::taskRunLoop(), I would probably make helper functions.)

[Sihui Liu]

Created attachment 384106 [details]
Patch

[Geoffrey Garen]

Comment on attachment 384106 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=384106&action=review

r=me

> Source/WebCore/Modules/indexeddb/server/IDBServer.h:196
> +    HashSet<UniqueIDBDatabase*> m_allUniqueIDBDatabases;

It would be better to use a WeakHashSet. That way, you don't need registerDatabase / unregisterDatabase, and you have pointer safety by default.

> Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabase.h:273
> +    Lock m_backingStoreLock;

Would be nice to add a comment explaining that the purpose of this lock is to synchronize vs suspension.

[Sihui Liu]

Created attachment 384172 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 384172 [details]
Patch for landing

Clearing flags on attachment: 384172

Committed r252787: <https://trac.webkit.org/changeset/252787>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/57434641>