Bug 110820

Summary: IndexedDB: Bind lifetime of in-memory backing stores to IDBFactory backend
Product: WebKit Reporter: Joshua Bell <jsbell>
Component: New BugsAssignee: Joshua Bell <jsbell>
Status: RESOLVED FIXED    
Severity: Normal CC: alecflett, dgrogan, tony, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 110675, 110677, 111233, 111459    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch for landing none

Description Joshua Bell 2013-02-25 16:35:49 PST
IndexedDB: Bind lifetime of in-memory backing stores to IDBFactory backend
Comment 1 Joshua Bell 2013-02-25 16:39:04 PST
Created attachment 190154 [details]
Patch
Comment 2 Joshua Bell 2013-02-25 16:41:45 PST
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?
Comment 3 David Grogan 2013-02-25 17:28:16 PST
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.
Comment 4 David Grogan 2013-02-25 18:23:34 PST
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())
Comment 5 Joshua Bell 2013-03-11 13:26:11 PDT
Created attachment 192540 [details]
Patch
Comment 6 Joshua Bell 2013-03-11 13:29:30 PDT
(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.
Comment 7 Joshua Bell 2013-03-11 13:33:53 PDT
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?
Comment 8 Joshua Bell 2013-03-19 11:04:27 PDT
Ping?
Comment 9 Joshua Bell 2013-03-29 10:16:31 PDT
Created attachment 195760 [details]
Patch
Comment 10 Joshua Bell 2013-03-29 10:34:57 PDT
(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?
Comment 11 Alec Flett 2013-03-29 11:02:45 PDT
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
Comment 12 Joshua Bell 2013-03-29 11:06:15 PDT
(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.
Comment 13 Joshua Bell 2013-03-29 11:06:35 PDT
tony@ - can you r?
Comment 14 Tony Chang 2013-03-29 11:39:30 PDT
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.
Comment 15 Joshua Bell 2013-03-29 11:42:53 PDT
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.
Comment 16 Joshua Bell 2013-03-29 11:48:31 PDT
Created attachment 195769 [details]
Patch for landing
Comment 17 WebKit Review Bot 2013-03-29 12:10:22 PDT
Comment on attachment 195769 [details]
Patch for landing

Clearing flags on attachment: 195769

Committed r147233: <http://trac.webkit.org/changeset/147233>
Comment 18 WebKit Review Bot 2013-03-29 12:10:27 PDT
All reviewed patches have been landed.  Closing bug.