Bug 79835 – IndexedDB: Resource leak with IDBObjectStore.openCursor

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Joshua Bell]

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.

[Joshua Bell]

Created attachment 129346 [details]
Patch

[David Grogan]

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?

[Joshua Bell]

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

[Joshua Bell]

Created attachment 129359 [details]
Patch

[Tony Chang]

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?

[Joshua Bell]

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

[Joshua Bell]

Created attachment 129480 [details]
Patch

[WebKit Review Bot]

Comment on attachment 129480 [details]
Patch

Clearing flags on attachment: 129480

Committed r109271: <http://trac.webkit.org/changeset/109271>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.