Bug 154015 - Modern IDB: Ref-cycles and leaks
Summary: Modern IDB: Ref-cycles and leaks
Status: ASSIGNED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: Safari 9
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brady Eidson
URL:
Keywords: InRadar
Depends on: 154032 154054 154061 154110 154187 158093 158632 158643 158694
Blocks: 165889
  Show dependency treegraph
 
Reported: 2016-02-08 16:02 PST by Brady Eidson
Modified: 2016-12-14 20:40 PST (History)
4 users (show)

See Also:


Attachments
Basic test with object store (1.38 KB, text/html)
2016-02-09 17:07 PST, Brady Eidson
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Brady Eidson 2016-02-08 16:02:41 PST
Modern IDB: Ref-cycles and leaks

IDBDatabase, IDBTransaction, IDBRequest, IDBObjectStore, IDBIndex, IDBCursor...  all ref each other in a big circle of ref cycling.

Let's fix, shall we?
Comment 1 Brady Eidson 2016-02-08 16:50:13 PST
I'll be exploring this bit by bit to come up with the best lifetime strategy and logging my findings here.

First finding that directs future exploration:
On a very basic test page that:
-opens a new database
-creates one object store
-lets the transaction finish
-navigates away to a new page

...The IDBOpenDBRequest remains alive with a ref-count of 1. The holder of that ref is the IDBTransaction that represents the version change transaction.

Additionally, there's a circular reference between the IDBOpenDBRequest and the IDBTransaction.

The transaction doesn't need the request anymore after the last event has fired, so that ref should be broken, which would break the ref on the transaction itself.
Comment 2 Brady Eidson 2016-02-09 14:04:26 PST
Next in line is IDBDatabase:

It has a couple of refs that come and go during event dispatch, all nicely balanced.

It's initial ref is held as the result of the OpenDBRequest. So as long as the open DB request is cleaned up, we're fine.

In the case of an upgrade needed, it's next ref is held by the version change IDBTransaction.

And, in fact, that ref lasts as long as the transaction object, which is "a very very long time."

To see if there's anything circular there, I'll now dig in to the transaction lifetime.
Comment 3 Brady Eidson 2016-02-09 16:10:30 PST
(In reply to comment #2)
> Next in line is IDBDatabase:
> 
> It has a couple of refs that come and go during event dispatch, all nicely
> balanced.
> 
> It's initial ref is held as the result of the OpenDBRequest. So as long as
> the open DB request is cleaned up, we're fine.
> 
> In the case of an upgrade needed, it's next ref is held by the version
> change IDBTransaction.
> 
> And, in fact, that ref lasts as long as the transaction object, which is "a
> very very long time."
> 
> To see if there's anything circular there, I'll now dig in to the
> transaction lifetime.

Transactions leak a couple of different ways.

One huge way is that TransactionOperations leak! Yikes. That one is an easy fix.

https://bugs.webkit.org/show_bug.cgi?id=154054 for that
Comment 4 Brady Eidson 2016-02-09 16:55:18 PST
(In reply to comment #3)
> (In reply to comment #2)
> > Next in line is IDBDatabase:
> > 
> > It has a couple of refs that come and go during event dispatch, all nicely
> > balanced.
> > 
> > It's initial ref is held as the result of the OpenDBRequest. So as long as
> > the open DB request is cleaned up, we're fine.
> > 
> > In the case of an upgrade needed, it's next ref is held by the version
> > change IDBTransaction.
> > 
> > And, in fact, that ref lasts as long as the transaction object, which is "a
> > very very long time."
> > 
> > To see if there's anything circular there, I'll now dig in to the
> > transaction lifetime.
> 
> Transactions leak a couple of different ways.
> 
> One huge way is that TransactionOperations leak! Yikes. That one is an easy
> fix.
> 
> https://bugs.webkit.org/show_bug.cgi?id=154054 for that

So with the fix for 154054, and with the previous fix for IDBOpenDBRequests, the most basic example of IDB usage works without leaks.

Attaching that "basic-test.html" that I've been using so far.

I'll start adding to it now to explore other options in the IDB cloud. (Non OpenDB requests, object stores, indexes, cursors...)
Comment 5 Brady Eidson 2016-02-09 17:07:25 PST
Created attachment 270964 [details]
Basic test with object store

This is NOT the basic test I mentioned - it's the basic test + one created object store.
Comment 6 Brady Eidson 2016-02-09 17:08:08 PST
(In reply to comment #5)
> Created attachment 270964 [details]
> Basic test with object store
> 
> This is NOT the basic test I mentioned - it's the basic test + one created
> object store.

And, no surprise, creating the object store re-introduces some ref cycles.

The object store refs the transaction, so the transaction doesn't die, and the transaction refs the database, so it doesn't die either.
Comment 7 Brady Eidson 2016-02-09 17:11:46 PST
(In reply to comment #6)
> (In reply to comment #5)
> > Created attachment 270964 [details]
> > Basic test with object store
> > 
> > This is NOT the basic test I mentioned - it's the basic test + one created
> > object store.
> 
> And, no surprise, creating the object store re-introduces some ref cycles.
> 
> The object store refs the transaction, so the transaction doesn't die, and
> the transaction refs the database, so it doesn't die either.

The IDBObjectStore needs to keep a reference to the transaction (IDBObjectStore.transaction), and the transaction has to keep references to all of its object stores for IDBTransaction.objectStore(<name>);

BUT - It only needs to do so until it's finished.

And I doubt it clears out those references.

Checking now...
Comment 8 Brady Eidson 2016-02-09 21:35:29 PST
(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > Created attachment 270964 [details]
> > > Basic test with object store
> > > 
> > > This is NOT the basic test I mentioned - it's the basic test + one created
> > > object store.
> > 
> > And, no surprise, creating the object store re-introduces some ref cycles.
> > 
> > The object store refs the transaction, so the transaction doesn't die, and
> > the transaction refs the database, so it doesn't die either.
> 
> The IDBObjectStore needs to keep a reference to the transaction
> (IDBObjectStore.transaction), and the transaction has to keep references to
> all of its object stores for IDBTransaction.objectStore(<name>);
> 
> BUT - It only needs to do so until it's finished.
> 
> And I doubt it clears out those references.
> 
> Checking now...

This was easy to resolve for object stores (https://bugs.webkit.org/show_bug.cgi?id=154061)

But the link between object stores and their indexes will be much more difficult to break.

As long as the object store is reachable by script, it has to keep all of its referenced indexes alive. Similarly, as long as an index is reachable by script, it has to keep its object store alive.

That one will be harder to break.
Comment 9 Brady Eidson 2016-02-09 21:36:33 PST
Note for future self: If we come across some cycle that's "impossible" to break, like possibly the one above, we can at the very least leverage ActiveDOMObject and break them all forcefully when ::stop() is called.
Comment 10 Brady Eidson 2016-02-11 13:36:47 PST
(In reply to comment #9)
> Note for future self: If we come across some cycle that's "impossible" to
> break, like possibly the one above, we can at the very least leverage
> ActiveDOMObject and break them all forcefully when ::stop() is called.

This one is definitely possible to break with a great strategy presented by Geoff.

But implementing it perfectly is proving problematic.

Covered by https://bugs.webkit.org/show_bug.cgi?id=154110
Comment 11 Brady Eidson 2016-04-22 20:16:54 PDT
While I hope to get back to exploring more leaks soon, and Darin has expressed interest in exploring them as well, there's still a big elephant in the room: Testing.

The strategy implement by Google back when V8 was in the project was "observeGC"

It went as follows:
    objectObserver = internals.observeGC(object);
    object = null;

    gc();

    shouldBeTrue("objectObserver");

If we had exactly this, that would be fine.
If we had any other mechanism that allowed for notification or inspection as to whether or not a particular object was GC'ed, that would be fine as well.

Whichever we go with, it seems extremely likely we'd need direct JSC support.
Comment 12 Brady Eidson 2016-05-25 13:42:37 PDT
https://bugs.webkit.org/show_bug.cgi?id=158004 likely introduced some more leaks from Workers
Comment 13 Brady Eidson 2016-05-25 14:54:38 PDT
I've been given a hint on how to implement observeGC-like functionality using existing JSC API.

Filed https://bugs.webkit.org/show_bug.cgi?id=158093 for that
Comment 14 Brady Eidson 2016-07-20 08:30:25 PDT
<rdar://problem/27445937>