Created attachment 129333 [details] Repro case Run attached page. It will "leak" 123 of each of: IDBRequest: 123 IDBTransaction: 123 IDBCursorWithValue: 123 IDBTransactionBackendImpl: 123 IDBCursor: 123 IDBKey: 123 IDBObjectStore: 123 IDBCursorBackendImpl: 123 (The backend objects are reclaimed on app shutdown, so it's not a true memory leak.) After setting up a database with a single store, and (necessarily) a single record, the script runs: function runtest(db) { for (var i = 0; i < 123; ++i) { db.transaction('store', webkitIDBTransaction.READ_ONLY).objectStore('store').openCursor(); } done(); } done() just runs an explicit GC call to clean up temporary objects that aren't being leaked. The leak goes away if either (1) there are no records in the store, or (2) openCursor() is not called.
Created attachment 129346 [details] Patch
Comment on attachment 129346 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=129346&action=review LGTM Hopefully this ref-counting spaghetti gets a bit untangled in idb.next. > Source/WebCore/storage/IDBCursor.cpp:117 > + if (m_request && m_request->resetReadyState(m_transaction.get())) { Similar question to below; m_request would only eval to false if continue() is called after close(), which I wouldn't have thought possible. Are there situations where that happens? > Source/WebCore/storage/IDBCursor.cpp:143 > + if (m_request) { This check seems to indicate that close can be called twice. When does that happen?
(In reply to comment #2) > (From update of attachment 129346 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=129346&action=review > > LGTM > > Hopefully this ref-counting spaghetti gets a bit untangled in idb.next. > > > Source/WebCore/storage/IDBCursor.cpp:117 > > + if (m_request && m_request->resetReadyState(m_transaction.get())) { > > Similar question to below; m_request would only eval to false if continue() is called after close(), which I wouldn't have thought possible. Are there situations where that happens? This can happen if the user retains a reference to the IDBCursor object but the transaction has finished. Once the transaction finishes, m_request will be cleared out (the new behavior). Previously, it would have called through to IDBCursorBackendImpl::continueFunction which would try and schedule a task against the transaction, which would fail with IDBDatabaseException::TRANSACTION_INACTIVE_ERR So... I should update this to fail early with that error instead if m_request is null. I'll add a test to verify the error. > > Source/WebCore/storage/IDBCursor.cpp:143 > > + if (m_request) { > > This check seems to indicate that close can be called twice. When does that happen? Ah, you're right, I can take that out.
Created attachment 129359 [details] Patch
Comment on attachment 129359 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=129359&action=review > Source/WebCore/storage/IDBCursor.cpp:58 > + m_transaction->registerOpenCursor(this); Is it possible to use a C++ class with scoping for this so you don't have to remember to call unregisterOpenCursor manually? > Source/WebCore/storage/IDBTransaction.cpp:144 > + for (HashSet<IDBCursor*>::iterator i = cursors.begin(); i != cursors.end(); ++i) > + (*i)->close(); Does calling closeOpenCursors cause unregisterOpenCursor to be called? I guess at that point m_openCursors is an empty HashSet and remove just ignores the call?
(In reply to comment #5) > (From update of attachment 129359 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=129359&action=review > > > Source/WebCore/storage/IDBCursor.cpp:58 > > + m_transaction->registerOpenCursor(this); > > Is it possible to use a C++ class with scoping for this so you don't have to remember to call unregisterOpenCursor manually? > Yep. I was blindly following the pattern used in IDBTransactionBackendImpl but I agree RAII-style is cleaner. I'll update the patch. > > Source/WebCore/storage/IDBTransaction.cpp:144 > > + for (HashSet<IDBCursor*>::iterator i = cursors.begin(); i != cursors.end(); ++i) > > + (*i)->close(); > > Does calling closeOpenCursors cause unregisterOpenCursor to be called? I guess at that point m_openCursors is an empty HashSet and remove just ignores the call? It depends on whether there are requests in flight or if script holds any references. I used the swap pattern so such calls are safe.
Created attachment 129480 [details] Patch
Comment on attachment 129480 [details] Patch Clearing flags on attachment: 129480 Committed r109271: <http://trac.webkit.org/changeset/109271>
All reviewed patches have been landed. Closing bug.