Bug 75859 - IndexedDB: IDBDatabase destructor is never executed
Summary: IndexedDB: IDBDatabase destructor is never executed
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: jochen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-01-09 08:21 PST by jochen
Modified: 2012-11-20 04:07 PST (History)
9 users (show)

See Also:


Attachments
Patch (2.16 KB, patch)
2012-01-10 02:12 PST, jochen
no flags Details | Formatted Diff | Diff
Patch (6.50 KB, patch)
2012-01-10 03:43 PST, jochen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description jochen 2012-01-09 08:21:25 PST
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.
Comment 1 jochen 2012-01-09 08:42:10 PST
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?
Comment 2 Joshua Bell 2012-01-09 10:18:04 PST
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.
Comment 3 jochen 2012-01-09 10:58:15 PST
but IDBTransaction is a RefPtr to IDBDatabase, shouldn't that be enough?
Comment 4 Joshua Bell 2012-01-09 17:29:09 PST
(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!)
Comment 5 jochen 2012-01-10 02:12:33 PST
Created attachment 121812 [details]
Patch
Comment 6 jochen 2012-01-10 02:14:23 PST
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.
Comment 7 Tony Gentilcore 2012-01-10 02:23:20 PST
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.
Comment 8 Tony Gentilcore 2012-01-10 02:30:44 PST
(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).
Comment 9 jochen 2012-01-10 03:43:45 PST
Created attachment 121822 [details]
Patch
Comment 10 Joshua Bell 2012-01-10 11:00:50 PST
(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.
Comment 11 jochen 2012-01-10 11:13:03 PST
(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
Comment 12 Joshua Bell 2012-01-11 17:34:29 PST
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.
Comment 13 WebKit Review Bot 2012-01-13 06:58:42 PST
Comment on attachment 121822 [details]
Patch

Clearing flags on attachment: 121822

Committed r104933: <http://trac.webkit.org/changeset/104933>
Comment 14 WebKit Review Bot 2012-01-13 06:58:46 PST
All reviewed patches have been landed.  Closing bug.
Comment 15 Adam Barth 2012-11-20 02:02:10 PST
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).
Comment 16 Adam Barth 2012-11-20 02:03:41 PST
tonyg and/or jochen, can you explain to me what keeps the IDBDatabase's wrapper alive while there is a pending transaction?
Comment 17 jochen 2012-11-20 04:07:11 PST
(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.