154015 2016-02-08 16:02:41 -0800 Modern IDB: Ref-cycles and leaks 2016-12-14 20:40:50 -0800 1 1 1 Unclassified WebKit WebCore Misc. Safari 9 Unspecified Unspecified ASSIGNED https://bugs.webkit.org/show_bug.cgi?id=158004 InRadar P2 Normal --- 154032 154054 154061 154110 154187 158093 158632 158643 158694 165889 1 beidson beidson achristensen ap darin ggaren oldest_to_newest 1163049 0 beidson 2016-02-08 16:02:41 -0800 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? 1163061 1 beidson 2016-02-08 16:50:13 -0800 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. 1163280 2 beidson 2016-02-09 14:04:26 -0800 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. 1163322 3 beidson 2016-02-09 16:10:30 -0800 (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 1163345 4 beidson 2016-02-09 16:55:18 -0800 (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...) 1163347 5 270964 beidson 2016-02-09 17:07:25 -0800 Created attachment 270964 Basic test with object store This is NOT the basic test I mentioned - it's the basic test + one created object store. 1163349 6 beidson 2016-02-09 17:08:08 -0800 (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. 1163351 7 beidson 2016-02-09 17:11:46 -0800 (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... 1163385 8 beidson 2016-02-09 21:35:29 -0800 (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. 1163386 9 beidson 2016-02-09 21:36:33 -0800 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. 1163847 10 beidson 2016-02-11 13:36:47 -0800 (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 1186563 11 beidson 2016-04-22 20:16:54 -0700 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. 1196445 12 beidson 2016-05-25 13:42:37 -0700 https://bugs.webkit.org/show_bug.cgi?id=158004 likely introduced some more leaks from Workers 1196512 13 beidson 2016-05-25 14:54:38 -0700 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 1212739 14 beidson 2016-07-20 08:30:25 -0700 <rdar://problem/27445937> 270964 2016-02-09 17:07:25 -0800 2016-02-09 17:07:25 -0800 Basic test with object store basic-test.html text/html 1414 beidson PHNjcmlwdD4KCmZ1bmN0aW9uIGdjKCkgewogICAgaWYgKHR5cGVvZiBHQ0NvbnRyb2xsZXIgIT09 ICJ1bmRlZmluZWQiKQogICAgICAgIEdDQ29udHJvbGxlci5jb2xsZWN0KCk7CiAgICBlbHNlIHsK ICAgICAgICB2YXIgZ2NSZWMgPSBmdW5jdGlvbiAobikgewogICAgICAgICAgICBpZiAobiA8IDEp CiAgICAgICAgICAgICAgICByZXR1cm4ge307CiAgICAgICAgICAgIHZhciB0ZW1wID0ge2k6ICJh YiIgKyBpICsgKGkgLyAxMDAwMDApfTsKICAgICAgICAgICAgdGVtcCArPSAiZm9vIjsKICAgICAg ICAgICAgZ2NSZWMobi0xKTsKICAgICAgICB9OwogICAgICAgIGZvciAodmFyIGkgPSAwOyBpIDwg MTAwMDsgaSsrKQogICAgICAgICAgICBnY1JlYygxMCkKICAgIH0KfQoKZnVuY3Rpb24gbG9nKG1z ZykKewogICAgZG9jdW1lbnQuZ2V0RWxlbWVudEJ5SWQoImxvZ2dlciIpLmlubmVySFRNTCArPSBt c2cgKyAiPGJyPiI7Cn0KCnZhciBvcGVuUmVxdWVzdCA9IHdpbmRvdy5pbmRleGVkREIub3Blbigi VGVzdERhdGFiYXNlIik7Cm9wZW5SZXF1ZXN0Lm9udXBncmFkZW5lZWRlZCA9IHByZXBhcmVEYXRh YmFzZTsKCnZhciBvYmplY3RTdG9yZTsKCmZ1bmN0aW9uIHByZXBhcmVEYXRhYmFzZShldmVudCkK ewogICAgb2JqZWN0U3RvcmUgPSBldmVudC50YXJnZXQudHJhbnNhY3Rpb24uZGIuY3JlYXRlT2Jq ZWN0U3RvcmUoImhlbGxvIik7CgogICAgZXZlbnQudGFyZ2V0LnRyYW5zYWN0aW9uLm9uYWJvcnQg PSBmdW5jdGlvbihldmVudCkgewogICAgICAgIGxvZygiSW5pdGlhbCB1cGdyYWRlIHZlcnNpb25j aGFuZ2UgdHJhbnNhY3Rpb24gdW5leHBlY3RlZCBhYm9ydGVkIik7CiAgICB9CgogICAgZXZlbnQu dGFyZ2V0LnRyYW5zYWN0aW9uLm9uY29tcGxldGUgPSBmdW5jdGlvbihldmVudCkgewogICAgICAg IGxvZygiSW5pdGlhbCB1cGdyYWRlIHZlcnNpb25jaGFuZ2UgdHJhbnNhY3Rpb24gY29tcGxldGUi KTsKICAgIH0KCiAgICBldmVudC50YXJnZXQudHJhbnNhY3Rpb24ub25lcnJvciA9IGZ1bmN0aW9u KGV2ZW50KSB7CiAgICAgICAgbG9nKCJJbml0aWFsIHVwZ3JhZGUgdmVyc2lvbmNoYW5nZSB0cmFu c2FjdGlvbiBlcnJvciAiICsgZXZlbnQpOwogICAgfQp9CgpmdW5jdGlvbiBjbGlja2VkKCkKewog ICAgb3BlblJlcXVlc3QgPSBudWxsOwogICAgb2JqZWN0U3RvcmUgPSBudWxsOwp9Cgo8L3Njcmlw dD4KPGJvZHk+CjxhIGhyZWY9Imh0dHA6Ly93d3cuYnJhZGVlb2guY29tLyI+Q2xpY2sgZm9yIEI8 L2E+PGJyPjxicj4KPGJ1dHRvbiBvbmNsaWNrPSJjbGlja2VkKCk7Ij5DbGljayB0byBjbGVhciBz dHVmZjwvYnV0dG9uPjxicj4KPGJ1dHRvbiBvbmNsaWNrPSJnYygpOyI+Q2xpY2sgdG8gR0M8L2J1 dHRvbj4KPGRpdiBpZD0ibG9nZ2VyIj48L2Rpdj4KPC9ib2R5Pgo8L2h0bWw+Cg==