Bug 46823

Created attachment 69223 [details] Patch

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.

One more comment: > oid IDBTransactionBackendImpl::abort() This needs to abort the timer. My bug.

(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.

Created attachment 69335 [details] Patch

Comment on attachment 69335 [details] Patch rubberstamp after discussion with andrei

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