Indexed DB transactions outdated immediately after it just created

Reproduction:

Note: idb is a famous library that wrap IndexedDB API into Promise style to make it easier to use.

It does not contains high level abstractions so it is likely not the bug of idb itself.

For reference, this issue is also tracked in the https://github.com/jakearchibald/idb/issues/201

Expected result on Chrome and Firefox:
https://user-images.githubusercontent.com/5390719/93753730-358a2900-fc33-11ea-80a5-b7d4f3eb731e.png

Bad result on Safari:
https://user-images.githubusercontent.com/5390719/93753660-12f81000-fc33-11ea-94c5-7771dab160c2.png

```js
import('https://cdn.skypack.dev/idb@5.0.6/with-async-ittr').then(async (idb) => {
    await idb.deleteDB('test')
    const sleep = (ms) => new Promise((resolve) => setTimeout(resolve, ms))
    const openDB = (() => {
        /** @type {import('idb').IDBPDatabase<unknown>} */
        let db = undefined
        return async () => {
            if (db) return db
            if (!db)
                db = await idb.openDB('test', 1, {
                    upgrade(db, oldVersion, newVersion, transaction) {
                        db.createObjectStore('store')
                    },
                })
            db.addEventListener('close', () => (db = undefined))
            return db
        }
    })()
    /** @type {import('idb').IDBPTransaction<unknown, ["store"]>} */
    let transaction = undefined
    const beforeTx = async () => {
        try {
            await transaction.objectStore('store').openCursor(IDBKeyRange.only('a'))
            console.log('The transaction is alive!')
        } catch (e) {
            console.log('Transaction outdated', e, 'creating new one')
            transaction = (await openDB()).transaction('store', 'readwrite')
        }
    }
    await beforeTx()
    await transaction.store.add({ a: 1 }, 'a')
    console.log('added a')
    await beforeTx()
    await transaction.store.add({ a: 1 }, 'b')
    console.log('added b')

    await sleep(120)
    await beforeTx()
    await transaction.store.add({ a: 1 }, 'c')
    console.log('added c')

    await sleep(120)
    console.log('before call beforeTx')
    await beforeTx()
    console.log('after call beforeTx')
    const cursor = await transaction.store.openCursor(IDBKeyRange.only('a'))
    console.log(cursor.value)
    const next = await cursor.continue()
    console.log(next)
})
```

This issues happens on Safari 12 to 14.0 (15610.1.28.1.9, 15610)

[Radar WebKit Bug Importer]

<rdar://problem/69321075>

[Sihui Liu]

(In reply to zjwpeter from comment #1)
> This issues happens on Safari 12 to 14.0 (15610.1.28.1.9, 15610)

Hi, 

Are you referring to the exception thrown after sleep(120)?

The behavior seems correct according to spec:
https://w3c.github.io/IndexedDB/#transaction-finished, "automatic committing". Transaction is supposed to be short-lived, and it will be automatically if there is no pending request and no request is to be created synchronously.

Hi I understand this idea of transaction auto commits.
You can see I use a function to check if the transaction has been outdated every time before I start a transaction operation.
My check do work, if the transaction has been outdated, it will create a new transaction immediately.

After the new transaction created there is no any synchronous operation, on the chrome and Firefox, this work well. But on the safari it seems like it is possible that the transaction is outdated immediately just after it has been created.

Please carefully check my example again, I didn't include a wrong usage of indexed DB transaction.

[Sihui Liu]

(In reply to zjwpeter from comment #4)
> Hi I understand this idea of transaction auto commits.
> You can see I use a function to check if the transaction has been outdated
> every time before I start a transaction operation.
> My check do work, if the transaction has been outdated, it will create a new
> transaction immediately.
> 
> After the new transaction created there is no any synchronous operation, on
> the chrome and Firefox, this work well. But on the safari it seems like it
> is possible that the transaction is outdated immediately just after it has
> been created.
> 
> Please carefully check my example again, I didn't include a wrong usage of
> indexed DB transaction.

ah sorry, I misunderstood your question. You meant 
await transaction.store.add({ a: 1 }, 'c')
fails as newly created transaction by beforeTx() is inactive. This seems a real bug.

Specifically, we are deactivating transaction when vm is idle:
vm.whenIdle([protectedThis = makeRef(*this)]() {
    protectedThis->deactivate();
});

I don't know why we need this, changelog does not provide explanation either: https://trac.webkit.org/changeset/198762/webkit. Maybe we should remove this.

[Sihui Liu]

(In reply to Sihui Liu from comment #5)
> (In reply to zjwpeter from comment #4)
> > Hi I understand this idea of transaction auto commits.
> > You can see I use a function to check if the transaction has been outdated
> > every time before I start a transaction operation.
> > My check do work, if the transaction has been outdated, it will create a new
> > transaction immediately.
> > 
> > After the new transaction created there is no any synchronous operation, on
> > the chrome and Firefox, this work well. But on the safari it seems like it
> > is possible that the transaction is outdated immediately just after it has
> > been created.
> > 
> > Please carefully check my example again, I didn't include a wrong usage of
> > indexed DB transaction.
> 
> ah sorry, I misunderstood your question. You meant 
> await transaction.store.add({ a: 1 }, 'c')
> fails as newly created transaction by beforeTx() is inactive. This seems a
> real bug.
> 
> Specifically, we are deactivating transaction when vm is idle:
> vm.whenIdle([protectedThis = makeRef(*this)]() {
>     protectedThis->deactivate();
> });
> 
> I don't know why we need this, changelog does not provide explanation
> either: https://trac.webkit.org/changeset/198762/webkit. Maybe we should
> remove this.

Another look at the spec today, https://w3c.github.io/IndexedDB/#transaction-inactive
says transaction is inactive when control returns to event loop. For this test case, IIUC, transaction being and records being added are two tasks, i.e. execute JS -> JS VM idle(deactivating transaction) -> execute JS... in this perspective, it seems right to deactivate the transaction :|. @Brady may know more as he initially added this.

> Another look at the spec today, https://w3c.github.io/IndexedDB/#transaction-inactive
> says transaction is inactive when control returns to event loop. For this test case, IIUC, transaction being and records being added are two tasks, i.e. execute JS -> JS VM idle(deactivating transaction) -> execute JS... in this perspective, it seems right to deactivate the transaction :|. @Brady may know more as he initially added this.


It says "A transaction is in this state(inactive) after control returns to the event loop after its creation". But Promises and async function are not join the event loop, they are microtasks.

> A microtask is a short function which is executed after the function or program which created it exits and only if the JavaScript execution stack is empty, but before returning control to the event loop being used by the user agent to drive the script's execution environment

(https://developer.mozilla.org/en-US/docs/Web/API/HTML_DOM_API/Microtask_guide)

[Sihui Liu]

(In reply to zjwpeter from comment #7)
> > Another look at the spec today, https://w3c.github.io/IndexedDB/#transaction-inactive
> > says transaction is inactive when control returns to event loop. For this test case, IIUC, transaction being and records being added are two tasks, i.e. execute JS -> JS VM idle(deactivating transaction) -> execute JS... in this perspective, it seems right to deactivate the transaction :|. @Brady may know more as he initially added this.
> 
> 
> It says "A transaction is in this state(inactive) after control returns to
> the event loop after its creation". But Promises and async function are not
> join the event loop, they are microtasks.
> 
> > A microtask is a short function which is executed after the function or program which created it exits and only if the JavaScript execution stack is empty, but before returning control to the event loop being used by the user agent to drive the script's execution environment
> 
> (https://developer.mozilla.org/en-US/docs/Web/API/HTML_DOM_API/
> Microtask_guide)

Thanks for the info. I confirmed transaction being created is in the same runloop task with record being added, so VM::whenIdle is definitely not what should be used here, as it does not tell us when control is returned.

[Sihui Liu]

Created attachment 410245 [details]
Patch

[Sihui Liu]

Created attachment 410249 [details]
Patch

[Sihui Liu]

Created attachment 410252 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 410252 [details]
Patch

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

> Source/WebCore/dom/EventLoop.cpp:71
> +void EventLoop::queueIndexedDBTask(TaskFunction&& function)

I wouldn't call this IndexedDBTask.
Rather, I'd call this queueWorkAtEndOfCurrentTask or something.

> Source/WebCore/dom/EventLoop.cpp:73
> +    m_indexedDBTaskFunctions.append(WTFMove(function));

It's possible for a task to be executing outside of EventLoop::run so we'd need to schedule it here.
Just call scheduleToRunIfNeeded

> Source/WebCore/dom/EventLoop.cpp:79
> +    auto functions = std::exchange(m_indexedDBTaskFunctions, { });
> +    for (auto& function : functions)

This isn't right. Some of these functions may be associated with a page that has gotten into a page cache or otherwise stopped.

> Source/WebCore/dom/EventLoop.cpp:180
> +    m_eventLoop->queueIndexedDBTask(WTFMove(function));

We need to create EventLoopTask here and associate that task with this group.

[Ryosuke Niwa]

Comment on attachment 410252 [details]
Patch

Don't set r- on your own patch. The right thing to do is to remove r?.

[Sihui Liu]

Created attachment 422706 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 422706 [details]
Patch

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

> Source/WebCore/Modules/indexeddb/IDBTransaction.cpp:101
> +        context->eventLoop().completeAfterMicrotaskCheckpoint([protectedThis = makeRef(*this)] {

I'd call this runAtEndOfMicrotaskCheckpoint instead.

> Source/WebCore/dom/EventLoop.cpp:189
> +    m_functionsAfterMicrotaskCheckpoint.append(WTFMove(function));
> +    microtaskQueue().addCheckpointListener(makeWeakPtr(this));

I'd preferred if we just had this mapping in EventLoop itself so that we preserve ordering
between functions coming from different groups.
Otherwise, we may inadvertently introduce a race condition depending on
whether this task group had already been scheduled on the event loop or not.

[Sihui Liu]

Created attachment 422842 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 422842 [details]
Patch

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

> LayoutTests/storage/indexeddb/transaction-state-active-after-creation.html:7
> +<script src="resources/transaction-state-active-after-creation.js"></script>

Can we just inline the script? It's old style of js-test to split the main testing script into a separate file.

[Sihui Liu]

Created attachment 422907 [details]
Patch for landing

[EWS]

Committed r274269: <https://commits.webkit.org/r274269>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 422907 [details].

[Sihui Liu]

*** Bug 222746 has been marked as a duplicate of this bug. ***