Bug 104615 – [Chromium] IndexedDB: Memory leak in IDBCallbacksProxy::onSuccess(PassRefPtr<IDBDatabaseBackendInterface>)

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Joshua Bell]

https://code.google.com/p/chromium/issues/detail?id=122249

Valgrind says:

{
   bug_122249d
   Memcheck:Leak
   fun:_Znw*
   fun:_ZN6WebKit17IDBCallbacksProxy9onSuccessEN3WTF10PassRefPtrIN7WebCore27IDBDatabaseBackendInterfaceEEE
   fun:_ZN7WebCore22IDBDatabaseBackendImpl19processPendingCallsEv
   fun:_ZN7WebCore22IDBDatabaseBackendImpl35transactionFinishedAndCompleteFiredEN3WTF10PassRefPtrINS_25IDBTransactionBackendImplEEE
   fun:_ZN7WebCore25IDBTransactionBackendImpl6commitEv
}

Leak is pretty obvious, here's a speculative fix but I haven't run it through valgrind yet:

 void WebIDBCallbacksImpl::onSuccess(WebIDBDatabase* webKitInstance)
 {
+    OwnPtr<WebIDBDatabase> webDatabase = adoptPtr(webKitInstance);
     if (m_databaseProxy) {
         m_callbacks->onSuccess(m_databaseProxy.release());
         return;
     }
-    m_callbacks->onSuccess(IDBDatabaseBackendProxy::create(adoptPtr(webKitInstance)));
+    m_callbacks->onSuccess(IDBDatabaseBackendProxy::create(webDatabase.release()));
 }

[Joshua Bell]

Created attachment 178674 [details]
Patch

[Joshua Bell]

dgrogan@, alecflett@ - please take a look

Verified this addresses the leak w/ valgrind, at least in basics.html

[David Grogan]

Comment on attachment 178674 [details]
Patch

LGTM

[Joshua Bell]

tony@ - r?

[WebKit Review Bot]

Comment on attachment 178674 [details]
Patch

Clearing flags on attachment: 178674

Committed r137330: <http://trac.webkit.org/changeset/137330>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by bug 104719

[Joshua Bell]

Created attachment 179521 [details]
Patch

[Joshua Bell]

The previous patch was on the receiving end, and didn't account for the "real" Chromium implementation which correct passed in the right objects. For the Chromium-DRT implementation, simply don't generate a new wrapper object if one was sent through in onUpgradeNeeded, and let it be ignored.

dgrogan@ - another look?

[David Grogan]

In chromium, won't this cause

IndexedDBCallbacksDatabase::onSuccess(WebKit::WebIDBDatabase* idb_object)

to attempt to delete NULL?

Also, to ensure I understand, this leak only happens in DRT, correct? Not chromium?

[Joshua Bell]

(In reply to comment #10)
> In chromium, won't this cause
> 
> IndexedDBCallbacksDatabase::onSuccess(WebKit::WebIDBDatabase* idb_object)
> 
> to attempt to delete NULL?

Correct (but delete null is a safe no-op in C++)
 
> Also, to ensure I understand, this leak only happens in DRT, correct? Not chromium?

Correct. (Well, Chromium's DRT to be precise, it wouldn't affect other ports.)

[David Grogan]

LGTM

[Joshua Bell]

tony@ - patch is completely different, so can you take another look?

[WebKit Review Bot]

Comment on attachment 179521 [details]
Patch

Clearing flags on attachment: 179521

Committed r137773: <http://trac.webkit.org/changeset/137773>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.