WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
ASSIGNED
154015
Modern IDB: Ref-cycles and leaks
https://bugs.webkit.org/show_bug.cgi?id=154015
Summary
Modern IDB: Ref-cycles and leaks
Brady Eidson
Reported
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?
Attachments
Basic test with object store
(1.38 KB, text/html)
2016-02-09 17:07 PST
,
Brady Eidson
no flags
Details
View All
Add attachment
proposed patch, testcase, etc.
Brady Eidson
Comment 1
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.
Brady Eidson
Comment 2
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.
Brady Eidson
Comment 3
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
Brady Eidson
Comment 4
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...)
Brady Eidson
Comment 5
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.
Brady Eidson
Comment 6
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.
Brady Eidson
Comment 7
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...
Brady Eidson
Comment 8
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.
Brady Eidson
Comment 9
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.
Brady Eidson
Comment 10
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
Brady Eidson
Comment 11
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.
Brady Eidson
Comment 12
2016-05-25 13:42:37 PDT
https://bugs.webkit.org/show_bug.cgi?id=158004
likely introduced some more leaks from Workers
Brady Eidson
Comment 13
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
Brady Eidson
Comment 14
2016-07-20 08:30:25 PDT
<
rdar://problem/27445937
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug