Bug 83074

Created attachment 135444 [details] Patch

dgrogan@, can you take a look and see if this makes sense before I send it off for formal review? The WebCore ChangeLog comment includes my understanding of the object ownership graph.

Comment on attachment 135444 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=135444&action=review LGTM I went through our dependency graph; it's amazing we haven't had more problems with leaks or use-after-frees. Though I guess we have had quite a number. > Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:123 > + if (m_factory) Can you add a comment here saying that this check should only be triggered in tests? > Source/WebKit/chromium/tests/IDBDatabaseBackendTest.cpp:42 > +class MockIDBBackingStore : public IDBBackingStore { Some bike-shedding: Mock means that it is going to verify that some set of operations were called on it. This should be Fake or Stub.

Created attachment 135936 [details] Patch

(In reply to comment #3) > > Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:123 > > + if (m_factory) > > Can you add a comment here saying that this check should only be triggered in tests? Done. > > Source/WebKit/chromium/tests/IDBDatabaseBackendTest.cpp:42 > > +class MockIDBBackingStore : public IDBBackingStore { > > Some bike-shedding: Mock means that it is going to verify that some set of operations were called on it. This should be Fake or Stub. Done.

tony@ - would you take a look?

Oh, and for the record: this refactor part of the work to enable the DB connection to be closed to unlock the backing store on disk so it can be deleted at the user's request, regardless of lingering object references on the JS side.

Comment on attachment 135936 [details] Patch Clearing flags on attachment: 135936 Committed r113473: <http://trac.webkit.org/changeset/113473>

All reviewed patches have been landed. Closing bug.

Reverted r113473 for reason: Change does not handle cursor retention of LevelDB iterators Committed r113622: <http://trac.webkit.org/changeset/113622>

Created attachment 142055 [details] Patch

dgrogan@ - take a look at the latest patch (It includes https://bugs.webkit.org/show_bug.cgi?id=86499 and requires https://chromiumcodereview.appspot.com/10382180 for the tests to pass in DRT)

Comment on attachment 142055 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=142055&action=review I'd break this up into 3-4 patches: 1. Already exists in https://bugs.webkit.org/show_bug.cgi?id=86499 2. Tracing stuff 3. Change instances of index->m_backingStore to index->backingStore() that returns m_backingStore, same with index->m_databaseID, and possibly the finished -> isFinished change. 4. The rest. > Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:391 > + // This check should only be false in tests. Thanks for this comment. > Source/WebCore/Modules/indexeddb/IDBTracing.h:37 > +#define IDB_TRACE(a) TRACE_EVENT0("IndexedDB", (a)); LOG_ERROR((a)); Guessing this wasn't supposed to be included? > Source/WebKit/chromium/tests/IDBDatabaseBackendTest.cpp:42 > +class FakeIDBBackingStore : public IDBBackingStore { I will adapt, or just use, this for us in a webkit unit test I'm writing, thanks.

(In reply to comment #13) > (From update of attachment 142055 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=142055&action=review > > I'd break this up into 3-4 patches: > 1. Already exists in https://bugs.webkit.org/show_bug.cgi?id=86499 > 2. Tracing stuff ^^^ I wasn't actually going to include that in the final patch, it was just for understanding the code. Will remove it before really submitting for review. > 3. Change instances of index->m_backingStore to index->backingStore() that returns m_backingStore, same with index->m_databaseID, and possibly the finished -> isFinished change. Good idea. > 4. The rest. > > Source/WebCore/Modules/indexeddb/IDBTracing.h:37 > > +#define IDB_TRACE(a) TRACE_EVENT0("IndexedDB", (a)); LOG_ERROR((a)); > > Guessing this wasn't supposed to be included? Right, just temporary. > > Source/WebKit/chromium/tests/IDBDatabaseBackendTest.cpp:42 > > +class FakeIDBBackingStore : public IDBBackingStore { > > I will adapt, or just use, this for us in a webkit unit test I'm writing, thanks. Cool!

Created attachment 142568 [details] Patch

Created attachment 143351 [details] Patch

Comment on attachment 143351 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=143351&action=review > Source/WebCore/Modules/indexeddb/IDBDatabase.cpp:232 > + closeConnection(); Does this change behavior? Were there cases before where we closed while transactions were still running? I see that this is one of the cases you want to test. Perhaps before this patch there was an implicit mechanism that prevented a database from closing while there were still transactions? > Source/WebCore/Modules/indexeddb/IDBFactoryBackendImpl.cpp:135 > + m_databaseBackendMap.remove(uniqueIdentifier); Out of scope for this patch, but it looks like if someone deletes a non-existing database we create it *on disk* and then delete it. Is that true? > Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.h:29 > +#include "IDBDatabaseBackendImpl.h" I think you can get rid of this if you move the accessor methods to the cpp file. > Source/WebCore/Modules/indexeddb/IDBTransactionBackendImpl.cpp:134 > + // database to be fully close. closeOpenCursors has to happen before transactionFinished to avoid the deletion order problem we had with the last similar patch, correct? I don't understand why the location of the callbacks call matters, though. The script-supplied callback could call db.close()? Why does it matter if that happens before or after the call to transactionFinished()?

Comment on attachment 143351 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=143351&action=review >> Source/WebCore/Modules/indexeddb/IDBDatabase.cpp:232 >> + closeConnection(); > > Does this change behavior? Were there cases before where we closed while transactions were still running? I see that this is one of the cases you want to test. Perhaps before this patch there was an implicit mechanism that prevented a database from closing while there were still transactions? Not processing the close until all transactions have finished is explicitly in the IDB spec. IIRC our previous behavior was that a close would end up aborting running transactions rather than waiting for them to finish, so this represents an intentional behavior change. (I need to add tests to verify that *was* the previous behavior, though.) >> Source/WebCore/Modules/indexeddb/IDBFactoryBackendImpl.cpp:135 >> + m_databaseBackendMap.remove(uniqueIdentifier); > > Out of scope for this patch, but it looks like if someone deletes a non-existing database we create it *on disk* and then delete it. Is that true? I believe so. Go us. :) >> Source/WebCore/Modules/indexeddb/IDBTransactionBackendImpl.cpp:134 >> + // database to be fully close. > > closeOpenCursors has to happen before transactionFinished to avoid the deletion order problem we had with the last similar patch, correct? > > I don't understand why the location of the callbacks call matters, though. The script-supplied callback could call db.close()? Why does it matter if that happens before or after the call to transactionFinished()? Yes, and.... I'm not sure. In the diff it looks like an intentional change but I suspect it wasn't (i.e. some code was rearranged then re-ordered and that line's position changed *because* it didn't matter) I definitely need to go over this with a fine toothed comb again and finish those tests before calling this ready.

Comment on attachment 143351 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=143351&action=review >>> Source/WebCore/Modules/indexeddb/IDBDatabase.cpp:232 >>> + closeConnection(); >> >> Does this change behavior? Were there cases before where we closed while transactions were still running? I see that this is one of the cases you want to test. Perhaps before this patch there was an implicit mechanism that prevented a database from closing while there were still transactions? > > Not processing the close until all transactions have finished is explicitly in the IDB spec. IIRC our previous behavior was that a close would end up aborting running transactions rather than waiting for them to finish, so this represents an intentional behavior change. (I need to add tests to verify that *was* the previous behavior, though.) SG - I know it's in the spec, I should have mentioned that I like how it's now explicit in the code :) >>> Source/WebCore/Modules/indexeddb/IDBTransactionBackendImpl.cpp:134 >>> + // database to be fully close. >> >> closeOpenCursors has to happen before transactionFinished to avoid the deletion order problem we had with the last similar patch, correct? >> >> I don't understand why the location of the callbacks call matters, though. The script-supplied callback could call db.close()? Why does it matter if that happens before or after the call to transactionFinished()? > > Yes, and.... I'm not sure. In the diff it looks like an intentional change but I suspect it wasn't (i.e. some code was rearranged then re-ordered and that line's position changed *because* it didn't matter) > > I definitely need to go over this with a fine toothed comb again and finish those tests before calling this ready. Doesn't your comment refer to the order? I guess I didn't understand what the comment was talking about, not only why it matters.

Comment on attachment 143351 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=143351&action=review >>>> Source/WebCore/Modules/indexeddb/IDBTransactionBackendImpl.cpp:134 >>>> + // database to be fully close. >>> >>> closeOpenCursors has to happen before transactionFinished to avoid the deletion order problem we had with the last similar patch, correct? >>> >>> I don't understand why the location of the callbacks call matters, though. The script-supplied callback could call db.close()? Why does it matter if that happens before or after the call to transactionFinished()? >> >> Yes, and.... I'm not sure. In the diff it looks like an intentional change but I suspect it wasn't (i.e. some code was rearranged then re-ordered and that line's position changed *because* it didn't matter) >> >> I definitely need to go over this with a fine toothed comb again and finish those tests before calling this ready. > > Doesn't your comment refer to the order? I guess I didn't understand what the comment was talking about, not only why it matters. I believe the comment only applies to the closeOpenCursors() and m_transaction line which (as you mention) must occur first to address the deletion order problem. The m_database->transactionFinished(this); can probably move back down.

Created attachment 146358 [details] Patch

Created attachment 146380 [details] Patch

Created attachment 146386 [details] Patch

(In reply to comment #23) > Created an attachment (id=146386) [details] > Patch Sorry for the churn. Last patches include: * Added layout test to verify behavior of closing databases (all transactions are allowed to complete first, and closing unblocks version changes and deletes) * Rebase changes and tests * Restore the ordering of m_database->transactionFinished(this); calls per discussion here

Comment on attachment 146386 [details] Patch Attachment 146386 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12921392 New failing tests: http/tests/inspector/indexeddb/resources-panel.html http/tests/inspector/indexeddb/database-data.html storage/indexeddb/database-close.html http/tests/inspector/indexeddb/database-structure.html

Created attachment 146459 [details] Archive of layout-test-results from ec2-cr-linux-04 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-04 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Created attachment 146890 [details] Patch

This should be good to go except that it includes the fix in http://webkit.org/b/88788 as that is necessary for passing the tests. Once that has landed I'll rebase/re-up this patch. r? anyone?

Comment on attachment 146890 [details] Patch Attachment 146890 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12941406

(In reply to comment #29) > (From update of attachment 146890 [details]) > Attachment 146890 [details] did not pass mac-ews (mac): > Output: http://queues.webkit.org/results/12941406 Failure was in JSC binding for WebGL (JSOESTextureFloat) - unrelated.

Comment on attachment 146890 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=146890&action=review > LayoutTests/storage/indexeddb/database-close-expected.txt:42 > +FAIL code should be 6. Was 11. > +FAIL ename should be NotAllowedError. Was InvalidStateError. These are expected failures, right? You might want to add a FIXME to database-close.js about this.

(In reply to comment #31) > (From update of attachment 146890 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=146890&action=review > > > LayoutTests/storage/indexeddb/database-close-expected.txt:42 > > +FAIL code should be 6. Was 11. > > +FAIL ename should be NotAllowedError. Was InvalidStateError. > > These are expected failures, right? You might want to add a FIXME to database-close.js about this. No, not expected - they're the result of another patch that just landed which changed the codes to match the spec. I just noticed these myself and am re-running/updating expectations.

Created attachment 147160 [details] Patch for landing

Comment on attachment 147160 [details] Patch for landing Clearing flags on attachment: 147160 Committed r120131: <http://trac.webkit.org/changeset/120131>

All reviewed patches have been landed. Closing bug.