Bug 83074 – IndexedDB: ObjectStore/Index shouldn't hold reference to backing store

• Component: New Bugs
• Status: RESOLVED FIXED

[Joshua Bell]

IndexedDB: ObjectStore/Index shouldn't hold reference to backing store

[Joshua Bell]

Created attachment 135444 [details]
Patch

[Joshua Bell]

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.

[David Grogan]

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.

[Joshua Bell]

Created attachment 135936 [details]
Patch

[Joshua Bell]

(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.

[Joshua Bell]

tony@ - would you take a look?

[Joshua Bell]

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.

[WebKit Review Bot]

Comment on attachment 135936 [details]
Patch

Clearing flags on attachment: 135936

Committed r113473: <http://trac.webkit.org/changeset/113473>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Joshua Bell]

Reverted r113473 for reason:

Change does not handle cursor retention of LevelDB iterators

Committed r113622: <http://trac.webkit.org/changeset/113622>

[Joshua Bell]

Created attachment 142055 [details]
Patch

[Joshua Bell]

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)

[David Grogan]

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.

[Joshua Bell]

(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!

[Joshua Bell]

Created attachment 142568 [details]
Patch

[Joshua Bell]

Created attachment 143351 [details]
Patch

[David Grogan]

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()?

[Joshua Bell]

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.

[David Grogan]

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.

[Joshua Bell]

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.

[Joshua Bell]

Created attachment 146358 [details]
Patch

[Joshua Bell]

Created attachment 146380 [details]
Patch

[Joshua Bell]

Created attachment 146386 [details]
Patch

[Joshua Bell]

(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

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Joshua Bell]

Created attachment 146890 [details]
Patch

[Joshua Bell]

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?

[Build Bot]

Comment on attachment 146890 [details]
Patch

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

[Joshua Bell]

(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.

[Tony Chang]

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.

[Joshua Bell]

(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.

[Joshua Bell]

Created attachment 147160 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 147160 [details]
Patch for landing

Clearing flags on attachment: 147160

Committed r120131: <http://trac.webkit.org/changeset/120131>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.