Bug 75859 – IndexedDB: IDBDatabase destructor is never executed

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

The destructor of IDBDatabase is never executed: IDBDatabase always returns true for hasPendingActivity(), so it's never garbage collected during page lifetime. hasPendingActivity() is only false when the we navigate away from the page, but at this point (at least V8) doesn't garbage collect the objects anymore.

I don't understand the comment in hasPendingActivity. If there's something left that can generate an event with this IDBDatabase in it, shouldn't that something keep a refptr to the IDBDatabase?

[Joshua Bell]

I believe the scenario is one where all IDBDatabase references have been let go by script, but there's a transaction still running. With the current code the front-end isn't aware of pending activity since that lives on the back-end. 

We have some refactoring planned that should address that, but should keep the bug open.

but IDBTransaction is a RefPtr to IDBDatabase, shouldn't that be enough?

[Joshua Bell]

(In reply to comment #3)
> but IDBTransaction is a RefPtr to IDBDatabase, shouldn't that be enough?

It should be, unless the IDBTransaction has also been de-reference by script and there's nothing keeping *it* alive. Now that IDBTransaction tracks m_transactionFinished (recent addition) we probably can probably correct this. (Patch welcome!)

Created attachment 121812 [details]
Patch

I tried to write a LayoutTest, however, the object is first deleted when a garbage collection occurs, and there's no generic way to force a GC.

However, when manually running DRT with --js-flags=--expose-gc() and invoking gc(); I can verify that the object is destroyed.

[Tony Gentilcore]

Comment on attachment 121812 [details]
Patch

It actually looks like this will cause us to lose the fix applied by http://trac.webkit.org/changeset/78280 . I agree this looks like a real problem, but I suspect the solution is to find a smarter condition for hasPendingActivity rather than to remove it entirely. Alternatively, some explanation or justification for why this is the correct solution should be in the ChangeLog.

[Tony Gentilcore]

(In reply to comment #6)
> I tried to write a LayoutTest, however, the object is first deleted when a garbage collection occurs, and there's no generic way to force a GC.

Have you tried GCController.collect() ?

fast/dom/resources/script-element-gc.js is one example of its usage (grep around in LayoutTests for more).

Created attachment 121822 [details]
Patch

[Joshua Bell]

(In reply to comment #7)
> (From update of attachment 121812 [details])
> It actually looks like this will cause us to lose the fix applied by http://trac.webkit.org/changeset/78280 . I agree this looks like a real problem, but I suspect the solution is to find a smarter condition for hasPendingActivity rather than to remove it entirely. Alternatively, some explanation or justification for why this is the correct solution should be in the ChangeLog.

Yeah, see also https://bugs.webkit.org/show_bug.cgi?id=54146

We should not destruct if there's a path to the object e.g. from child objects (which RefPtrs should handle), if there's pending activity (which should be associated with an IDBTransaction, which has a RefPtr to the IDBDatabase), or if events can fire on the object.

For the latter, a call to EventTarget::hasEventListeners() might be sufficient.

(In reply to comment #10)
> (In reply to comment #7)
> > (From update of attachment 121812 [details] [details])
> > It actually looks like this will cause us to lose the fix applied by http://trac.webkit.org/changeset/78280 . I agree this looks like a real problem, but I suspect the solution is to find a smarter condition for hasPendingActivity rather than to remove it entirely. Alternatively, some explanation or justification for why this is the correct solution should be in the ChangeLog.
> 
> Yeah, see also https://bugs.webkit.org/show_bug.cgi?id=54146
> 
> We should not destruct if there's a path to the object e.g. from child objects (which RefPtrs should handle), if there's pending activity (which should be associated with an IDBTransaction, which has a RefPtr to the IDBDatabase), or if events can fire on the object.
> 
> For the latter, a call to EventTarget::hasEventListeners() might be sufficient.

IDBDatabase implements refEventTarget/derefEventTarget by ref/deref'ing itself, so that shouldn't be required

[Joshua Bell]

LGTM

I traced through the "close" logic again to satisfy myself that the IDBDatabase destructor will ensure the callbacks are cleared out not pointing back to the object.

[WebKit Review Bot]

Comment on attachment 121822 [details]
Patch

Clearing flags on attachment: 121822

Committed r104933: <http://trac.webkit.org/changeset/104933>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Adam Barth]

Comment on attachment 121822 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=121822&action=review

> Source/WebCore/ChangeLog:11
> +        connection. Meanwhile, transactions are marked as active DOM objects
> +        during their lifetime, and they keep a RefPtr to the IDBDatabase
> +        object, so this hack is no longer required.

This isn't sufficient.  If the underlying object is refed by the transactions but there are no JS references to the wrapper, then the wrapper can fall off (be garbage collected and then re-created later).

[Adam Barth]

tonyg and/or jochen, can you explain to me what keeps the IDBDatabase's wrapper alive while there is a pending transaction?

(In reply to comment #16)
> tonyg and/or jochen, can you explain to me what keeps the IDBDatabase's wrapper alive while there is a pending transaction?

That's actually a good question.