|Summary:||IndexedDB tests leak documents|
|Product:||WebKit||Reporter:||Simon Fraser (smfr) <simon.fraser>|
|Component:||WebKit Misc.||Assignee:||youenn fablet <youennf>|
|Severity:||Normal||CC:||alecflett, ap, beidson, cdumez, commit-queue, darin, drousso, esprehn+autocc, ews-watchlist, ggaren, joepeck, jsbell, kondapallykalyan, sihui_liu, simon.fraser, webkit-bug-importer, youennf|
|Version:||WebKit Nightly Build|
Description Simon Fraser (smfr) 2018-09-07 14:51:26 PDT
These tests are still leaking documents: imported/w3c/IndexedDB-private-browsing/idbcursor_delete_objectstore3.html [ Leak ] imported/w3c/IndexedDB-private-browsing/idbcursor_update_index3.html [ Leak ] imported/w3c/IndexedDB-private-browsing/idbcursor_update_objectstore3.html [ Leak ] imported/w3c/IndexedDB-private-browsing/value.html [ Leak ] imported/w3c/IndexedDB-private-browsing/value_recursive.html [ Leak ] imported/w3c/web-platform-tests/IndexedDB/idb-binary-key-roundtrip.htm [ Leak ] imported/w3c/web-platform-tests/IndexedDB/idbcursor-continuePrimaryKey-exception-order.htm [ Leak ] imported/w3c/web-platform-tests/IndexedDB/idbcursor-continuePrimaryKey.htm [ Leak ] imported/w3c/web-platform-tests/IndexedDB/idbcursor_delete_index3.htm [ Leak ] imported/w3c/web-platform-tests/IndexedDB/idbcursor_delete_objectstore3.htm [ Leak ] imported/w3c/web-platform-tests/IndexedDB/idbcursor_update_index3.htm [ Leak ] imported/w3c/web-platform-tests/IndexedDB/idbcursor_update_objectstore3.htm [ Leak ] imported/w3c/web-platform-tests/IndexedDB/idbindex-rename-abort.html [ Leak ] imported/w3c/web-platform-tests/IndexedDB/idbindex-rename-errors.html [ Leak ] imported/w3c/web-platform-tests/IndexedDB/idbindex-rename.html [ Leak ] imported/w3c/web-platform-tests/IndexedDB/idbindex_getAll.html [ Leak ] imported/w3c/web-platform-tests/IndexedDB/idbobjectstore-rename-abort.html [ Leak ] imported/w3c/web-platform-tests/IndexedDB/idbobjectstore-rename-errors.html [ Leak ] imported/w3c/web-platform-tests/IndexedDB/idbobjectstore-rename-store.html [ Leak ] imported/w3c/web-platform-tests/IndexedDB/key-conversion-exceptions.htm [ Leak ] imported/w3c/web-platform-tests/IndexedDB/keypath-special-identifiers.htm [ Leak ] imported/w3c/web-platform-tests/IndexedDB/request-abort-ordering.html [ Leak ] imported/w3c/web-platform-tests/IndexedDB/request-event-ordering.html [ Leak ] imported/w3c/IndexedDB-private-browsing/idbcursor_delete_index3.html [ Pass Leak ] imported/w3c/IndexedDB-private-browsing/transaction-requestqueue.html [ Pass Leak ] storage/indexeddb/closed-cursor-private.html [ Leak ] storage/indexeddb/closed-cursor.html [ Leak ] storage/indexeddb/create-object-store-options-private.html [ Leak ] storage/indexeddb/create-object-store-options.html [ Leak ] storage/indexeddb/cursor-properties-private.html [ Leak ] storage/indexeddb/cursor-properties.html [ Leak ] storage/indexeddb/cursor-value-private.html [ Leak ] storage/indexeddb/cursor-value.html [ Leak ] storage/indexeddb/deleted-objects-private.html [ Leak ] storage/indexeddb/deleted-objects.html [ Leak ] storage/indexeddb/exceptions-private.html [ Leak ] storage/indexeddb/exceptions.html [ Leak ] storage/indexeddb/key-type-array-private.html [ Leak ] storage/indexeddb/key-type-array.html [ Leak ] storage/indexeddb/keypath-arrays-private.html [ Leak ] storage/indexeddb/keypath-arrays.html [ Leak ] storage/indexeddb/keypath-fetch-key-private.html [ Leak ] storage/indexeddb/keypath-fetch-key.html [ Leak ] storage/indexeddb/modern/blob-svg-image.html [ Leak ] storage/indexeddb/modern/idbcursor-continue-primary-key-1-private.html [ Leak ] storage/indexeddb/modern/idbcursor-continue-primary-key-1.html [ Leak ] storage/indexeddb/modern/idbindex-getall-1-private.html [ Leak ] storage/indexeddb/modern/idbindex-getall-1.html [ Leak ] storage/indexeddb/modern/idbindex-getallkeys-1-private.html [ Leak ] storage/indexeddb/modern/idbindex-getallkeys-1.html [ Leak ] storage/indexeddb/modern/idbobjectstore-getall-1-private.html [ Leak ] storage/indexeddb/modern/idbobjectstore-getall-1.html [ Leak ] storage/indexeddb/modern/idbobjectstore-getallkeys-1-private.html [ Leak ] storage/indexeddb/modern/idbobjectstore-getallkeys-1.html [ Leak ] storage/indexeddb/mozilla/autoincrement-indexes-private.html [ Leak ] storage/indexeddb/mozilla/autoincrement-indexes.html [ Leak ] storage/indexeddb/mozilla/cursor-update-updates-indexes-private.html [ Leak ] storage/indexeddb/mozilla/cursor-update-updates-indexes.html [ Leak ] storage/indexeddb/mozilla/object-cursors-private.html [ Leak ] storage/indexeddb/mozilla/object-cursors.html [ Leak ] storage/indexeddb/mozilla/object-store-inline-autoincrement-key-added-on-put-private.html [ Leak ] storage/indexeddb/mozilla/object-store-inline-autoincrement-key-added-on-put.html [ Leak ] storage/indexeddb/mozilla/readyState-private.html [ Leak ] storage/indexeddb/mozilla/readyState.html [ Leak ] storage/indexeddb/objectstore-autoincrement-private.html [ Leak ] storage/indexeddb/objectstore-autoincrement.html [ Leak ] storage/indexeddb/optional-arguments-private.html [ Leak ] storage/indexeddb/optional-arguments.html [ Leak ] storage/indexeddb/prefetch-bugfix-108071-private.html [ Leak ] storage/indexeddb/prefetch-bugfix-108071.html [ Leak ] storage/indexeddb/readonly-private.html [ Leak ] storage/indexeddb/readonly.html [ Leak ]
Comment 4 youenn fablet 2019-02-06 18:11:18 PST
Comment on attachment 361349 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=361349&action=review > Source/WebCore/Modules/indexeddb/IDBCursor.h:109 > + JSValueInWrappedObject m_cachedValue; The patch is going from JSC::String to JSC::Weak. The patch is probably making sure that as long as the JSIDBCursor is alive, we will not GC these values. Can there be cases where: - we create a JSIDBCursor from an IDBCursor, GC the JSIDBCursor and recreate a new JSIDBCursor from the same IDBCursor. - we want to keep the cached values from being GCed when the JSIDBCursor is no longer there. Using DOMGuardedObject might be a solution so that we ensure that these values do not get GCed while making sure we do not create unbreakable ref cycles.
Comment 6 Sihui Liu 2019-02-07 09:43:41 PST
(In reply to youenn fablet from comment #4) > Comment on attachment 361349 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=361349&action=review > > > Source/WebCore/Modules/indexeddb/IDBCursor.h:109 > > + JSValueInWrappedObject m_cachedValue; > > The patch is going from JSC::String to JSC::Weak. > The patch is probably making sure that as long as the JSIDBCursor is alive, > we will not GC these values. > Can there be cases where: > - we create a JSIDBCursor from an IDBCursor, GC the JSIDBCursor and recreate > a new JSIDBCursor from the same IDBCursor. This is possible. Why is this a problem? > - we want to keep the cached values from being GCed when the JSIDBCursor is > no longer there. If there is JSIDBRequest, we will keep the cached value from being GCed. Other than that, do we need to keep the cached values around without having JSIDBCursor? This patch keeps previous behavior.
Comment 7 youenn fablet 2019-02-07 10:02:51 PST
> If there is JSIDBRequest, we will keep the cached value from being GCed. > Other than that, do we need to keep the cached values around without having > JSIDBCursor? That is my question, yes. I am not familiar enough in IDB to know whether this is ok or not. Code inspection or tests exercising IDB API and GC might help. > This patch keeps previous behavior. Not really, the previous behavior with JSC::Strong is to tie cached values lifetime with IDBCursor lifetime. This patch is tieing these cached values lifetime with JSIDBCursor lifetime.
Comment 8 Sihui Liu 2019-02-07 10:39:41 PST
(In reply to youenn fablet from comment #7) > > If there is JSIDBRequest, we will keep the cached value from being GCed. > > Other than that, do we need to keep the cached values around without having > > JSIDBCursor? > > That is my question, yes. > I am not familiar enough in IDB to know whether this is ok or not. > Code inspection or tests exercising IDB API and GC might help. > The change doesn't break any existing test, so far. > > This patch keeps previous behavior. > > Not really, the previous behavior with JSC::Strong is to tie cached values > lifetime with IDBCursor lifetime. This patch is tieing these cached values > lifetime with JSIDBCursor lifetime. Yes, by "keeping previous behavior" I mean keeping expected behavior from layout tests. Tying cached values to IDBCursor caused unexpected document leaks.
Comment 9 Geoffrey Garen 2019-02-07 11:06:09 PST
Comment 10 Geoffrey Garen 2019-02-07 11:13:31 PST
Comment 11 Geoffrey Garen 2019-02-07 11:18:41 PST
I don't think DOMGuardedObject is the right solution in this case because it would cause every IDB request and cursor to survive as long as the webpage survives. A webpage that executes lots of IDB queries will see a serious memory leak. In fact, it seems likely to me that many uses of DOMGuardedObject are memory leaks.
Comment 12 Geoffrey Garen 2019-02-07 11:25:50 PST
Consider DOMGuardedObject, I do think I see a pre-existing bug here, which I'd like you to fix in a separate bug: JSValueInWrappedObject neglects to execute any write barrier. JSValueInWrappedObject needs to tell the garbage collector that its owner now points to its value. Otherwise, the garbage collector, if it has already concurrently or generationally visited the owner, may neglect to visit the value. This is probably as simple as: (1) Call heap.writeBarrier(), passing owner and value, inside cachedPropertyValue, after we assign to cachedValue. (2) Change the JSValueInWrappedObject(JSC::JSValue value) to require an owner argument, and execute a write barrier there too.
Comment 15 WebKit Commit Bot 2019-02-08 09:05:59 PST
Comment on attachment 361483 [details] Patch Clearing flags on attachment: 361483 Committed r241196: <https://trac.webkit.org/changeset/241196>
Comment 16 WebKit Commit Bot 2019-02-08 09:06:01 PST
All reviewed patches have been landed. Closing bug.
Comment 17 Darin Adler 2019-02-08 13:05:16 PST
I don’t see any tests here or any test results changing. We need to create regression tests that cover the object lifetime, or be clearer about why that is difficult or impossible. When I created JSValueInWrappedObject, I created the reference-cycle-leaks test <https://trac.webkit.org/browser/webkit/trunk/LayoutTests/fast/dom/reference-cycle-leaks.html> and in writing the test was in fact more difficult than fixing the bug. There is a placeholder in there for an IDBRequest test, although not a placeholder for an IDBCursor test. I think it’s dangerous for us to say this is fixed without also adding a regression test, so we should settle for that only if we find it’s prohibitively difficult to do so. Also, since the comments above point to something wrong about the JSValueInWrappedObject implementation or possibly in how it’s being used, I also would like us to create tests that demonstrate that. We could do that either when we fix the problem, or, better, even before we fix the problem to help us understand even more deeply that the problem is real.
Comment 18 Darin Adler 2019-02-08 13:05:45 PST
I think seeing these indirectly through [ Leak ] isn’t the best way to test, but I suppose it’s better than nothing.
Comment 19 Geoffrey Garen 2019-02-08 22:18:13 PST
Seems like we could regression test this in a direct way by following the model of <https://trac.webkit.org/browser/webkit/trunk/LayoutTests/fast/dom/reference-cycle-leaks.html>: 1. Make a test with an iframe 2. Record window.internals.numberOfLiveDocuments() 3. Do 100 times: 3a. Navigate the iframe to a test page that uses IDBCursor 4. Record window.internals.numberOfLiveDocuments() - (2) Write barrier test will be hard. Maybe we can make it easier with a window.internals.performSyncEdenGC().
Comment 20 Darin Adler 2019-02-09 09:51:46 PST
One other side comment: The createPromiseRejectionEventPromiseCycle subtest of reference-cycle-leaks.html is still an expected failure.