Bug 100903 – IndexedDB: Move control of transaction completion to front end

• Component: New Bugs
• Status: RESOLVED FIXED

[Joshua Bell]

IndexedDB: Move control of transaction completion to front end

[Joshua Bell]

Created attachment 171742 [details]
Patch

[Joshua Bell]

Incomplete (and untested).

We may be able to simplify the IDBTransactionBackendImpl further:

* Eliminate the dual timers - one should be enough
* Eliminate the didCompleteTaskEvents message routing - the back end should no longer need to be aware of the status of the front end
* Subsume webkit.org/b/97738 and run multiple events per tick.

[Joshua Bell]

Extra complexity: asynchronous operations that don't have associated requests - i.e. store and index creation/deletion - are invisible to the front end.

The back end needs to wait for those to complete as well.

[Joshua Bell]

Created attachment 171945 [details]
Patch

[Joshua Bell]

Created attachment 173928 [details]
Patch

[Joshua Bell]

Comment on attachment 173928 [details]
Patch

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

A few explanatory notes (mostly to myself)

> Source/WebCore/Modules/indexeddb/IDBRequest.cpp:542
> +    if (m_transaction && m_readyState == DONE)

This is moved up so that the transaction's requestList is (possibly) empty when setActive(false) is called, so it can try and commit.

> Source/WebCore/Modules/indexeddb/IDBTransaction.cpp:99
> +    , m_state(Unfinished)

The distinction between Used and Unused was only to support triggering commits from the front end for unused transactions. Now the front-end is always responsible, so we can remove the distinction.

> Source/WebCore/Modules/indexeddb/IDBTransaction.cpp:206
> +    if (!active && m_requestList.isEmpty())

Here's the critical bit of the change.

> Source/WebCore/Modules/indexeddb/IDBTransactionBackendImpl.cpp:211
> +    m_commitPending = true;

The front end thinks it's done...

> Source/WebCore/Modules/indexeddb/IDBTransactionBackendImpl.cpp:213
> +    if (m_pendingEvents || m_pendingPreemptiveEvents || !isTaskQueueEmpty())

But the back end may be busy processing a createIndex so we need to wait.

> Source/WebCore/Modules/indexeddb/IDBTransactionBackendImpl.cpp:-208
> -    ASSERT(m_state == Unused || m_state == Running);

Moved up.

> Source/WebCore/Modules/indexeddb/IDBTransactionBackendImpl.cpp:-209
> -    ASSERT(isTaskQueueEmpty());

Implied by above if() test.

> Source/WebCore/Modules/indexeddb/IDBTransactionBackendImpl.cpp:276
> +    if (!m_pendingEvents && !m_pendingPreemptiveEvents && isTaskQueueEmpty()) {

Possible existing bug - commit might occur if there are pending pre-emptive events.

> Source/WebCore/Modules/indexeddb/IDBTransactionBackendImpl.cpp:277
> +        ASSERT(m_commitPending);

We shouldn't hit this state unless the front-end is already done.

[Alec Flett]

Comment on attachment 173928 [details]
Patch

this is awesome. lgtm with minor nits

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

>> Source/WebCore/Modules/indexeddb/IDBTransactionBackendImpl.cpp:213
>> +    if (m_pendingEvents || m_pendingPreemptiveEvents || !isTaskQueueEmpty())
> 
> But the back end may be busy processing a createIndex so we need to wait.

make this a comment - I think this check is pretty critical. (do we want to move this to a descriptive helper function?)

>> Source/WebCore/Modules/indexeddb/IDBTransactionBackendImpl.cpp:276
>> +    if (!m_pendingEvents && !m_pendingPreemptiveEvents && isTaskQueueEmpty()) {
> 
> Possible existing bug - commit might occur if there are pending pre-emptive events.

And this is just !(m_pendingEvents || m_pendingPreemptiveEvents || !isTaskQueueEmpty()) so I think a helper is justified.

[Joshua Bell]

(In reply to comment #7)
> 
> make this a comment - I think this check is pretty critical. (do we want to move this to a descriptive helper function?)

Good call. New patch coming.

Another thing we could consider (for later) is to have these "sync" operations (createObjectStore, createIndex) create "hidden" IDBRequests on the front end, so the lifetime is managed by the transaction just like regular async operations.

[Joshua Bell]

Created attachment 173955 [details]
Patch

[Alec Flett]

(In reply to comment #8)
> Another thing we could consider (for later) is to have these "sync" operations (createObjectStore, createIndex) create "hidden" IDBRequests on the front end, so the lifetime is managed by the transaction just like regular async operations.

+1

[Joshua Bell]

Created attachment 175029 [details]
Patch

[Joshua Bell]

Rebased. Also merged m_active (boolean) into m_state (enum) since the latter was changing anyway.

alecflett@ - another look please?

[Alec Flett]

Comment on attachment 175029 [details]
Patch

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

> Source/WebCore/Modules/indexeddb/IDBTransactionBackendImpl.cpp:51
> +    , m_commitPending(false)

How does m_commitPending work with m_state? I'm concerned (more from a code maintenance perspective than a correctness perspective) that we're now doubling the possible "states" - to the combinations of m_state and m_commitPending (even if most of the "new" ones are invalid) - i.e. is m_state Finished && m_commitPending = false possible? etc...

> Source/WebCore/Modules/indexeddb/IDBTransactionBackendImpl.cpp:127
> +        m_abortTaskQueue.removeFirst();

why this change?

[Joshua Bell]

Comment on attachment 175029 [details]
Patch

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

>> Source/WebCore/Modules/indexeddb/IDBTransactionBackendImpl.cpp:51
>> +    , m_commitPending(false)
> 
> How does m_commitPending work with m_state? I'm concerned (more from a code maintenance perspective than a correctness perspective) that we're now doubling the possible "states" - to the combinations of m_state and m_commitPending (even if most of the "new" ones are invalid) - i.e. is m_state Finished && m_commitPending = false possible? etc...

In theory orthogonal - can enter commitPending in parallel with Unused or Running. Logically can't enter commitPending when in StartPending because the front-end has queued tasks that haven't completed. This is covered by the ASSERT(m_state == Unused || m_state == Running) in ::commit().

Given that a commit() of an Unused transaction should be a no-op, I'll see if I can collapse it to an extra state "after" Running.

>> Source/WebCore/Modules/indexeddb/IDBTransactionBackendImpl.cpp:127
>> +        m_abortTaskQueue.removeFirst();
> 
> why this change?

Good catch. This is a rebase glitch. I'll undo.

[Joshua Bell]

(In reply to comment #14)
> Given that a commit() of an Unused transaction should be a no-op, I'll see if I can collapse it to an extra state "after" Running.

Bleah, it ends up being messier since commit() can be called more than once and the "unused" state is important for sending the correct notifications. However, m_commitPending was only used for ASSERT() so I'll just remove it.

[Joshua Bell]

Created attachment 175293 [details]
Patch

[Joshua Bell]

tony@ - r?

[WebKit Review Bot]

Comment on attachment 175293 [details]
Patch

Clearing flags on attachment: 175293

Committed r135332: <http://trac.webkit.org/changeset/135332>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.