Bug 110820 – IndexedDB: Bind lifetime of in-memory backing stores to IDBFactory backend

• Component: New Bugs
• Status: RESOLVED FIXED

[Joshua Bell]

IndexedDB: Bind lifetime of in-memory backing stores to IDBFactory backend

[Joshua Bell]

Created attachment 190154 [details]
Patch

[Joshua Bell]

Chromium bug: https://code.google.com/p/chromium/issues/detail?id=165311

The fix is straightforward; in the Chromium port, the lifetime of an IDBFactoryBackendImpl is tied to the lifetime of a "Context"; incognito mode creates a new context which would scope the lifetime of these memory-only backing stores.

Needs tests. Trivial to test on the chromium side as a content test (open/write/close/re-open/read); in WebKit-land will require a webkit_unit_test.

dgrogan@ - thoughts?

[David Grogan]

Comment on attachment 190154 [details]
Patch

LGTM

I think this is a fine solution to the incognito problem. You could consolidate the knowledge that an empty path indicates memory backing store by refactoring IDBBackingStore and creating an IDBBackingStore::openInMemory method.

I thought you had unit test somewhere that tests that backing stores are dropped when there are no more connections to it. It might be prudent to add a similar test for incognito backing stores that ensures the backing store is NOT dropped.  I guess you were referring to something similar in the changelog.

[David Grogan]

Comment on attachment 190154 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=190154&action=review

> Source/WebCore/Modules/indexeddb/IDBFactoryBackendImpl.cpp:138
> +    if (backingStore) {

Since incognito windows get their own IDBFactoryBackendImpl, you could
ASSERT(m_sessionOnlyBackingStores.isEmpty() || dataDirectory.isEmpty())

[Joshua Bell]

Created attachment 192540 [details]
Patch

[Joshua Bell]

(In reply to comment #2)
> The fix is straightforward; in the Chromium port, the lifetime of an IDBFactoryBackendImpl is tied to the lifetime of a "Context"; incognito mode creates a new context which would scope the lifetime of these memory-only backing stores.

Turned out to be trickier since it introduced a reference cycle; that was addressed in http://wkrev.com/145238

> Needs tests. Trivial to test on the chromium side as a content test (open/write/close/re-open/read); in WebKit-land will require a webkit_unit_test.

Added.

(In reply to comment #3)
> I think this is a fine solution to the incognito problem. You could consolidate the knowledge that an empty path indicates memory backing store by refactoring IDBBackingStore and creating an IDBBackingStore::openInMemory method.

Done.
 
(In reply to comment #4)
> Since incognito windows get their own IDBFactoryBackendImpl, you could
> ASSERT(m_sessionOnlyBackingStores.isEmpty() || dataDirectory.isEmpty())

Done.

[Joshua Bell]

Comment on attachment 192540 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=192540&action=review

dgrogan@, alecflett@ - can you take a look?

> Source/WebCore/Modules/indexeddb/IDBBackingStore.cpp:-385
> -    if (pathBase.isEmpty()) {

Sorry, the diff here is going to be hard to read...

> Source/WebCore/Modules/indexeddb/IDBBackingStore.cpp:-441
> -        HistogramSupport::histogramEnumeration("WebCore.IndexedDB.BackingStore.OpenStatus", IDBLevelDBBackingStoreOpenFailedUnknownErr, IDBLevelDBBackingStoreOpenMax);

Given the open/openInMemory split I ended up removing this block, since the code flow is simpler. Should I put it back?

[Joshua Bell]

Ping?

[Joshua Bell]

Created attachment 195760 [details]
Patch

[Joshua Bell]

(In reply to comment #9)
> Created an attachment (id=195760) [details]
> Patch

Rebased, and minimized the diff for IDBBackingStore.cpp - it's much easier to read if you apply the patch locally and diff with -w to hide the identation changes.

Alec - do you recall if we came up with any changes we wanted to make, or were we just discussing more lifetime assertions around stores vs. databases?

[Alec Flett]

Comment on attachment 195760 [details]
Patch

[We had discussed, but weren't able to resolve, the idea that a backing store should never outlive its factory, but we couldn't figure out a good way to assert if that was true or not, and we didn't even know if that was truly assertable because of shutdown ordering, so we punted]

lgtm

[Joshua Bell]

(In reply to comment #11)
> (From update of attachment 195760 [details])
> [We had discussed, but weren't able to resolve, the idea that a backing store should never outlive its factory, but we couldn't figure out a good way to assert if that was true or not, and we didn't even know if that was truly assertable because of shutdown ordering, so we punted]

Ah yes, thanks. The idea being that the IDBFactoryBackendImpl should in theory outlive any of the IDBBackingStores it produces. But given that the BackingStore->Factory pointer is now weak this may (in multi-process ports e.g. Chromium) rely on tear-down order of the  renderers that have opened connections (and thus have indirect IDBDatabase->proxy->IDBDatabaseBackend references) vs. the context associated with that renderer.

[Joshua Bell]

tony@ - can you r?

[Tony Chang]

Comment on attachment 195760 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=195760&action=review

> Source/WebCore/Modules/indexeddb/IDBBackingStore.cpp:354
> +#if PLATFORM(CHROMIUM)
> +    ASSERT(WebKit::Platform::current()->unitTestSupport());
> +#endif

I don't understand what this assert is checking.  When is unitTestSupport() null?

> Source/WebKit/chromium/tests/IDBBackingStoreTest.cpp:307
> +    EXPECT_EQ(memStore1->refCount(), 2);

Nit: I thought the expected values go on the left, but I guess this file gets this wrong in other places.

[Joshua Bell]

Comment on attachment 195760 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=195760&action=review

>> Source/WebCore/Modules/indexeddb/IDBBackingStore.cpp:354
>> +#endif
> 
> I don't understand what this assert is checking.  When is unitTestSupport() null?

This was suggested by abarth@ in another bug as a way of asserting "this should only be called/happen in unit tests".

https://bugs.webkit.org/show_bug.cgi?id=111233

I'll add a comment too, though.

>> Source/WebKit/chromium/tests/IDBBackingStoreTest.cpp:307
>> +    EXPECT_EQ(memStore1->refCount(), 2);
> 
> Nit: I thought the expected values go on the left, but I guess this file gets this wrong in other places.

I'll fix in this method at least.

[Joshua Bell]

Created attachment 195769 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 195769 [details]
Patch for landing

Clearing flags on attachment: 195769

Committed r147233: <http://trac.webkit.org/changeset/147233>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.