Bug 46823 – Finished IDBTransaction for IndexedDB

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jeremy Orlow]

Finished IDBTransaction for IndexedDB

[Jeremy Orlow]

Created attachment 69223 [details]
Patch

[Andrei Popescu]

Nice work, first round of comments:

> class SerializedScriptValue : public ThreadSafeShared<SerializedScriptValue> {

Hmm, this is a change that affects everyone so may be a perf issue for platforms that don't define USE_LOCKFREE_THREADSAFESHARED so now have to lock a mutex for each refcount.

>  if (!m_transaction->scheduleTask(createCallbackTask(&IDBCursorBackendImpl::updateInternal, this, value, callbacks)))

The type parameters for the CrossThreadTask constructor are inferred form this call, so the Task object will store a raw IDBCursorBackendImpl pointer. If that happens to be released before the task is executed, we'll crash due to a dangling pointer.

> scheduleTask(createCallbackTask(&IDBCursorBackendImpl::continueFunctionInternal, this, key, callbacks)))

ditto

> scheduleTask(createCallbackTask(&IDBCursorBackendImpl::removeInternal, this, callbacks)))

ditto

> scheduleTask(createCallbackTask(&IDBDatabaseBackendImpl::createObjectStoreInternal, this, name, keyPath, autoIncrement, callbacks)))

ditto

> scheduleTask(createCallbackTask(&IDBDatabaseBackendImpl::removeObjectStoreInternal, this, name, callbacks)))

ditto

> scheduleTask(createCallbackTask(&openCursorInternal, this, keyRange, direction, true, callbacks, transaction))

ditto

> scheduleTask(createCallbackTask(&openCursorInternal, this, keyRange, direction, false, callbacks, transaction)))

ditto

> scheduleTask(createCallbackTask(&getInternal, this, key, true, callbacks)))

ditto

> scheduleTask(createCallbackTask(&getInternal, this, key, false, callbacks)))

ditto

> scheduleTask(createCallbackTask(&IDBObjectStoreBackendImpl::getInternal, this, key, callbacks)))

ditto


> RefPtr<IDBRequest> request = IDBRequest::create(context, IDBAny::create(this), m_setVersionTransaction.get());
> 62     if (!m_setVersionTransaction)
> 63         request->onError(IDBDatabaseError::create(IDBDatabaseException::NOT_ALLOWED_ERR,
> "createObjectStore must be called from within a setVersion transaction."));

Nice but it won't work after merging with 46883, as we lose the callbacks param. I'm updating that patch to make this throw an exception instead.

[Andrei Popescu]

One more comment:

> oid IDBTransactionBackendImpl::abort()

This needs to abort the timer. My bug.

[Jeremy Orlow]

(In reply to comment #2)
> Nice work, first round of comments:
> 
> > class SerializedScriptValue : public ThreadSafeShared<SerializedScriptValue> {
> 
> Hmm, this is a change that affects everyone so may be a perf issue for platforms that don't define USE_LOCKFREE_THREADSAFESHARED so now have to lock a mutex for each refcount.

I too was worried about this, so I did some research: It's mostly just used for history push state, indexedDB, message ports, and event source.  None of them should have too much ref count thrashing (they use pointers in many cases).  So I'm pretty sure this won't cause a regression.

If we're super worried or see performance issues, we could probably leave it single threaded and simply copy the underlying string.

[Jeremy Orlow]

Created attachment 69335 [details]
Patch

[Steve Block]

Comment on attachment 69335 [details]
Patch

rubberstamp after discussion with andrei

[Jeremy Orlow]

Committed r68795: <http://trac.webkit.org/changeset/68795>