Bug 164974

Created attachment 295640 [details] Patch

This caught two patches in the upload, whoops. Sending only one now...

Created attachment 295642 [details] Patch

Comment on attachment 295642 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=295642&action=review r=me > Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:2383 > + return m_cachedStatements[static_cast<int>(sql)].get(); This should be bounds checked just to be extra careful, with an ASSERT_NOT_REACHED of some sort. Also, size_t would be more appropriate than int.

Comment on attachment 295642 [details] Patch Attachment 295642 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/2591094 Number of test failures exceeded the failure limit.

Created attachment 295652 [details] Archive of layout-test-results from ews105 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5

Those failures are due to the classic "w3c web server stopped working" issue.

This caused issues under Guardmalloc with deadlocked TransactionOperations. Landing a stopgap fix until I can further explore why they aren't ref'ed

(In reply to comment #9) > This caused issues under Guardmalloc with deadlocked TransactionOperations. > > Landing a stopgap fix until I can further explore why they aren't ref'ed Landed this in https://trac.webkit.org/changeset/209104

(In reply to comment #10) > (In reply to comment #9) > > This caused issues under Guardmalloc with deadlocked TransactionOperations. > > > > Landing a stopgap fix until I can further explore why they aren't ref'ed > > Landed this in https://trac.webkit.org/changeset/209104 This fix caused storage/indexeddb/modern/gc-closes-database.html and storage/indexeddb/modern/gc-closes-database-private.html to start timing out (I'm sure of why, also) I'll skip them until I can resolve this mess tomorrow.

(In reply to comment #11) > (In reply to comment #10) > > (In reply to comment #9) > > > This caused issues under Guardmalloc with deadlocked TransactionOperations. > > > > > > Landing a stopgap fix until I can further explore why they aren't ref'ed > > > > Landed this in https://trac.webkit.org/changeset/209104 > > This fix caused storage/indexeddb/modern/gc-closes-database.html and > storage/indexeddb/modern/gc-closes-database-private.html to start timing out > (I'm sure of why, also) > > I'll skip them until I can resolve this mess tomorrow. Skipped in https://trac.webkit.org/changeset/209114 Resolving the GuardMalloc *and* leak issue will happen over in bug https://bugs.webkit.org/show_bug.cgi?id=165186

Reverted r209096, r209104, and r209114 for reason: Caused over 200 layout test failures on the bots Committed r209117: <http://trac.webkit.org/changeset/209117>

Reverted r209096, r209104, and r209114 for reason: Caused over 200 layout test failures on the bots Committed r209117: <http://trac.webkit.org/changeset/209117>

(In reply to comment #14) > Reverted r209096, r209104, and r209114 for reason: > > Caused over 200 layout test failures on the bots > > Committed r209117: <http://trac.webkit.org/changeset/209117> E.g. https://build.webkit.org/builders/Apple%20El%20Capitan%20Release%20WK2%20%28Tests%29/builds/11350

(In reply to comment #15) > (In reply to comment #14) > > Reverted r209096, r209104, and r209114 for reason: > > > > Caused over 200 layout test failures on the bots > > > > Committed r209117: <http://trac.webkit.org/changeset/209117> > > E.g. > https://build.webkit.org/builders/ > Apple%20El%20Capitan%20Release%20WK2%20%28Tests%29/builds/11350 Note that https://trac.webkit.org/changeset/209104 did not help fix those layout test failures.

(In reply to comment #16) > (In reply to comment #15) > > (In reply to comment #14) > > > Reverted r209096, r209104, and r209114 for reason: > > > > > > Caused over 200 layout test failures on the bots > > > > > > Committed r209117: <http://trac.webkit.org/changeset/209117> > > > > E.g. > > https://build.webkit.org/builders/ > > Apple%20El%20Capitan%20Release%20WK2%20%28Tests%29/builds/11350 > > Note that https://trac.webkit.org/changeset/209104 did not help fix those > layout test failures. It wasn't meant to help fix those, which I didn't even know were happening. It *was* meant to fix GuardMalloc failures from a previous changeset that I guess was now reintroduced. It's somewhat surprising this change caused all that, but whatever - I'll sort it out in the A.M.

(In reply to comment #17) > (In reply to comment #16) > > (In reply to comment #15) > > > (In reply to comment #14) > > > > Reverted r209096, r209104, and r209114 for reason: > > > > > > > > Caused over 200 layout test failures on the bots > > > > > > > > Committed r209117: <http://trac.webkit.org/changeset/209117> > > > > > > E.g. > > > https://build.webkit.org/builders/ > > > Apple%20El%20Capitan%20Release%20WK2%20%28Tests%29/builds/11350 > > > > Note that https://trac.webkit.org/changeset/209104 did not help fix those > > layout test failures. > > It wasn't meant to help fix those, which I didn't even know were happening. > > It *was* meant to fix GuardMalloc failures from a previous changeset that I > guess was now reintroduced. I rolled out all 3 changesets so I do not think I reintroduced failures but I could be wrong. > > It's somewhat surprising this change caused all that, but whatever - I'll > sort it out in the A.M. Looks like the roll out was successful on the bots: - Before: https://build.webkit.org/builders/Apple%20Yosemite%20Release%20WK2%20%28Tests%29/builds/19663 - After: https://build.webkit.org/builders/Apple%20Yosemite%20Release%20WK2%20%28Tests%29/builds/19664 The remaining unit test failure was caused by another change which was rolled out later on and we are green as of: https://build.webkit.org/builders/Apple%20Yosemite%20Release%20WK2%20%28Tests%29/builds/19665

I can reproduce locally, only with the w3c tests, after a certain number of tests have run in a given process. e.g.: run-webkit-tests --child-processes=1 imported/w3c/web-platform-tests/IndexedDB/idbcursor_update_index3.htm --iterations=20000 Hammers the same test over and over, and after 68 runs it starts failing. lsof confirms tons of handles to databases from past tests are still open, so opening new database files fails (limit to the number of file handles per process) I'm still unclear how this change caused leaking file handles, but there's not much surface to go over.

(In reply to comment #19) > I can reproduce locally, only with the w3c tests, after a certain number of > tests have run in a given process. > > e.g.: > run-webkit-tests --child-processes=1 > imported/w3c/web-platform-tests/IndexedDB/idbcursor_update_index3.htm > --iterations=20000 > > Hammers the same test over and over, and after 68 runs it starts failing. > > lsof confirms tons of handles to databases from past tests are still open, > so opening new database files fails (limit to the number of file handles per > process) > > I'm still unclear how this change caused leaking file handles, but there's > not much surface to go over. It's also quite unclear why this *doesn't* affect DRT, but only WKTR. o_O

(In reply to comment #20) > (In reply to comment #19) > > I can reproduce locally, only with the w3c tests, after a certain number of > > tests have run in a given process. > > > > e.g.: > > run-webkit-tests --child-processes=1 > > imported/w3c/web-platform-tests/IndexedDB/idbcursor_update_index3.htm > > --iterations=20000 > > > > Hammers the same test over and over, and after 68 runs it starts failing. > > > > lsof confirms tons of handles to databases from past tests are still open, > > so opening new database files fails (limit to the number of file handles per > > process) > > > > I'm still unclear how this change caused leaking file handles, but there's > > not much surface to go over. > > It's also quite unclear why this *doesn't* affect DRT, but only WKTR. o_O DRT is leaking the handles, also. This must be "app process" vs "XPC service process" limitations. DRT *does* start failing after enough runs (850 instead of 68)

Okay figured it out - If you close a sqlite3 handle while you still have prepared statements open, it leaks the file handles. o_O Exact same patch with a couple of sqlite cleanup calls moved around fixes this.

Created attachment 295730 [details] Patch

Comment on attachment 295730 [details] Patch Clearing flags on attachment: 295730 Committed r209144: <http://trac.webkit.org/changeset/209144>

All reviewed patches have been landed. Closing bug.