IndexedDB: Bind lifetime of in-memory backing stores to IDBFactory backend
authorjsbell@chromium.org <jsbell@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 29 Mar 2013 19:12:04 +0000 (19:12 +0000)
committerjsbell@chromium.org <jsbell@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 29 Mar 2013 19:12:04 +0000 (19:12 +0000)
https://bugs.webkit.org/show_bug.cgi?id=110820

Reviewed by Tony Chang.

Source/WebCore:

Backing stores are dropped as soon as all connections are closed. That makes sense for
disk-backed stores to free up memory (although there's a performance trade-off...). But
for memory-backed stores, the expected lifetime should match the lifetime of the factory
so that an open/write/close/re-open/read yields the written data.

Test: Chromium's webkit_unit_tests, IDBFactoryBackendTest.MemoryBackingStoreLifetime

* Modules/indexeddb/IDBBackingStore.cpp:
(WebCore::IDBBackingStore::IDBBackingStore): Pass comparator into constructor since it was always
assigned immediately afterwards anyway.
(WebCore::IDBBackingStore::open): Split into three parts - open() which is disk-backed...
(WebCore::IDBBackingStore::openInMemory): ...explit in-memory creation (previously: specified by empty path)
(WebCore::IDBBackingStore::create): ... and the common logic which calls the constructor.
* Modules/indexeddb/IDBBackingStore.h: Headers for the above.
* Modules/indexeddb/IDBFactoryBackendImpl.cpp:
(WebCore::IDBFactoryBackendImpl::openBackingStore): Add in-memory backing stores to dependent set.
* Modules/indexeddb/IDBFactoryBackendImpl.h: Add member to track dependent backing stores.

Source/WebKit/chromium:

* tests/IDBBackingStoreTest.cpp:
(WebCore::IDBBackingStoreTest::SetUp): Use openInMemory rather than empty path.
(WebCore::TEST): Added IDBFactoryBackendTest.MemoryBackingStoreLifetime to verify refs.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@147233 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog
Source/WebCore/Modules/indexeddb/IDBBackingStore.cpp
Source/WebCore/Modules/indexeddb/IDBBackingStore.h
Source/WebCore/Modules/indexeddb/IDBFactoryBackendImpl.cpp
Source/WebCore/Modules/indexeddb/IDBFactoryBackendImpl.h
Source/WebKit/chromium/ChangeLog
Source/WebKit/chromium/tests/IDBBackingStoreTest.cpp

index 4f22f80..a2713f0 100644 (file)
@@ -1,3 +1,28 @@
+2013-03-29  Joshua Bell  <jsbell@chromium.org>
+
+        IndexedDB: Bind lifetime of in-memory backing stores to IDBFactory backend
+        https://bugs.webkit.org/show_bug.cgi?id=110820
+
+        Reviewed by Tony Chang.
+
+        Backing stores are dropped as soon as all connections are closed. That makes sense for
+        disk-backed stores to free up memory (although there's a performance trade-off...). But
+        for memory-backed stores, the expected lifetime should match the lifetime of the factory
+        so that an open/write/close/re-open/read yields the written data.
+
+        Test: Chromium's webkit_unit_tests, IDBFactoryBackendTest.MemoryBackingStoreLifetime
+
+        * Modules/indexeddb/IDBBackingStore.cpp:
+        (WebCore::IDBBackingStore::IDBBackingStore): Pass comparator into constructor since it was always
+        assigned immediately afterwards anyway.
+        (WebCore::IDBBackingStore::open): Split into three parts - open() which is disk-backed...
+        (WebCore::IDBBackingStore::openInMemory): ...explit in-memory creation (previously: specified by empty path)
+        (WebCore::IDBBackingStore::create): ... and the common logic which calls the constructor.
+        * Modules/indexeddb/IDBBackingStore.h: Headers for the above.
+        * Modules/indexeddb/IDBFactoryBackendImpl.cpp:
+        (WebCore::IDBFactoryBackendImpl::openBackingStore): Add in-memory backing stores to dependent set.
+        * Modules/indexeddb/IDBFactoryBackendImpl.h: Add member to track dependent backing stores.
+
 2013-03-29  Nate Chapin  <japhet@chromium.org>
 
         ASSERT d->m_defersLoading != defers on detik.com and drive.google.com
index 662f38e..ccc4cdb 100644 (file)
@@ -43,6 +43,9 @@
 #include "LevelDBTransaction.h"
 #include "SecurityOrigin.h"
 #include "SharedBuffer.h"
+#if PLATFORM(CHROMIUM)
+#include <public/Platform.h>
+#endif
 #include <wtf/Assertions.h>
 
 namespace WebCore {
@@ -335,9 +338,10 @@ public:
     }
 };
 
-IDBBackingStore::IDBBackingStore(const String& identifier, PassOwnPtr<LevelDBDatabase> db)
+IDBBackingStore::IDBBackingStore(const String& identifier, PassOwnPtr<LevelDBDatabase> db, PassOwnPtr<LevelDBComparator> comparator)
     : m_identifier(identifier)
     , m_db(db)
+    , m_comparator(comparator)
     , m_weakFactory(this)
 {
 }
@@ -345,6 +349,10 @@ IDBBackingStore::IDBBackingStore(const String& identifier, PassOwnPtr<LevelDBDat
 IDBBackingStore::IDBBackingStore()
     : m_weakFactory(this)
 {
+    // This constructor should only be used in unit tests.
+#if PLATFORM(CHROMIUM)
+    ASSERT(WebKit::Platform::current()->unitTestSupport());
+#endif
 }
 
 IDBBackingStore::~IDBBackingStore()
@@ -378,65 +386,56 @@ PassRefPtr<IDBBackingStore> IDBBackingStore::open(SecurityOrigin* securityOrigin
 PassRefPtr<IDBBackingStore> IDBBackingStore::open(SecurityOrigin* securityOrigin, const String& pathBaseArg, const String& fileIdentifier, LevelDBFactory* levelDBFactory)
 {
     IDB_TRACE("IDBBackingStore::open");
+    ASSERT(!pathBaseArg.isEmpty());
     String pathBase = pathBaseArg;
 
     OwnPtr<LevelDBComparator> comparator = adoptPtr(new Comparator());
     OwnPtr<LevelDBDatabase> db;
 
-    if (pathBase.isEmpty()) {
-        db = LevelDBDatabase::openInMemory(comparator.get());
-        if (!db) {
-            LOG_ERROR("LevelDBDatabase::openInMemory failed.");
-            HistogramSupport::histogramEnumeration("WebCore.IndexedDB.BackingStore.OpenStatus", IDBLevelDBBackingStoreOpenMemoryFailed, IDBLevelDBBackingStoreOpenMax);
-            return PassRefPtr<IDBBackingStore>();
-        }
-        HistogramSupport::histogramEnumeration("WebCore.IndexedDB.BackingStore.OpenStatus", IDBLevelDBBackingStoreOpenMemorySuccess, IDBLevelDBBackingStoreOpenMax);
-    } else {
-        if (!pathBase.containsOnlyASCII())
+    if (!pathBase.containsOnlyASCII())
             HistogramSupport::histogramEnumeration("WebCore.IndexedDB.BackingStore.OpenStatus", IDBLevelDBBackingStoreOpenAttemptNonASCII, IDBLevelDBBackingStoreOpenMax);
-        if (!makeAllDirectories(pathBase)) {
-            LOG_ERROR("Unable to create IndexedDB database path %s", pathBase.utf8().data());
-            HistogramSupport::histogramEnumeration("WebCore.IndexedDB.BackingStore.OpenStatus", IDBLevelDBBackingStoreOpenFailedDirectory, IDBLevelDBBackingStoreOpenMax);
-            return PassRefPtr<IDBBackingStore>();
-        }
-
-        String path = pathByAppendingComponent(pathBase, securityOrigin->databaseIdentifier() + ".indexeddb.leveldb");
+    if (!makeAllDirectories(pathBase)) {
+        LOG_ERROR("Unable to create IndexedDB database path %s", pathBase.utf8().data());
+        HistogramSupport::histogramEnumeration("WebCore.IndexedDB.BackingStore.OpenStatus", IDBLevelDBBackingStoreOpenFailedDirectory, IDBLevelDBBackingStoreOpenMax);
+        return PassRefPtr<IDBBackingStore>();
+    }
 
-        db = levelDBFactory->openLevelDB(path, comparator.get());
-        if (db) {
-            bool known = false;
-            bool ok = isSchemaKnown(db.get(), known);
-            if (!ok) {
-                LOG_ERROR("IndexedDB had IO error checking schema, treating it as failure to open");
-                HistogramSupport::histogramEnumeration("WebCore.IndexedDB.BackingStore.OpenStatus", IDBLevelDBBackingStoreOpenFailedIOErrCheckingSchema, IDBLevelDBBackingStoreOpenMax);
-                db.clear();
-            } else if (!known) {
-                LOG_ERROR("IndexedDB backing store had unknown schema, treating it as failure to open");
-                HistogramSupport::histogramEnumeration("WebCore.IndexedDB.BackingStore.OpenStatus", IDBLevelDBBackingStoreOpenFailedUnknownSchema, IDBLevelDBBackingStoreOpenMax);
-                db.clear();
-            }
+    String path = pathByAppendingComponent(pathBase, securityOrigin->databaseIdentifier() + ".indexeddb.leveldb");
+
+    db = levelDBFactory->openLevelDB(path, comparator.get());
+    if (db) {
+        bool known = false;
+        bool ok = isSchemaKnown(db.get(), known);
+        if (!ok) {
+            LOG_ERROR("IndexedDB had IO error checking schema, treating it as failure to open");
+            HistogramSupport::histogramEnumeration("WebCore.IndexedDB.BackingStore.OpenStatus", IDBLevelDBBackingStoreOpenFailedIOErrCheckingSchema, IDBLevelDBBackingStoreOpenMax);
+            db.clear();
+        } else if (!known) {
+            LOG_ERROR("IndexedDB backing store had unknown schema, treating it as failure to open");
+            HistogramSupport::histogramEnumeration("WebCore.IndexedDB.BackingStore.OpenStatus", IDBLevelDBBackingStoreOpenFailedUnknownSchema, IDBLevelDBBackingStoreOpenMax);
+            db.clear();
         }
+    }
 
-        if (db)
-            HistogramSupport::histogramEnumeration("WebCore.IndexedDB.BackingStore.OpenStatus", IDBLevelDBBackingStoreOpenSuccess, IDBLevelDBBackingStoreOpenMax);
-        else {
-            LOG_ERROR("IndexedDB backing store open failed, attempting cleanup");
-            bool success = levelDBFactory->destroyLevelDB(path);
-            if (!success) {
-                LOG_ERROR("IndexedDB backing store cleanup failed");
-                HistogramSupport::histogramEnumeration("WebCore.IndexedDB.BackingStore.OpenStatus", IDBLevelDBBackingStoreOpenCleanupDestroyFailed, IDBLevelDBBackingStoreOpenMax);
-                return PassRefPtr<IDBBackingStore>();
-            }
+    if (db)
+        HistogramSupport::histogramEnumeration("WebCore.IndexedDB.BackingStore.OpenStatus", IDBLevelDBBackingStoreOpenSuccess, IDBLevelDBBackingStoreOpenMax);
+    else {
+        LOG_ERROR("IndexedDB backing store open failed, attempting cleanup");
+        bool success = levelDBFactory->destroyLevelDB(path);
+        if (!success) {
+            LOG_ERROR("IndexedDB backing store cleanup failed");
+            HistogramSupport::histogramEnumeration("WebCore.IndexedDB.BackingStore.OpenStatus", IDBLevelDBBackingStoreOpenCleanupDestroyFailed, IDBLevelDBBackingStoreOpenMax);
+            return PassRefPtr<IDBBackingStore>();
+        }
 
-            LOG_ERROR("IndexedDB backing store cleanup succeeded, reopening");
-            db = levelDBFactory->openLevelDB(path, comparator.get());
-            if (!db) {
-                LOG_ERROR("IndexedDB backing store reopen after recovery failed");
-                HistogramSupport::histogramEnumeration("WebCore.IndexedDB.BackingStore.OpenStatus", IDBLevelDBBackingStoreOpenCleanupReopenFailed, IDBLevelDBBackingStoreOpenMax);
-                return PassRefPtr<IDBBackingStore>();
-            }
-            HistogramSupport::histogramEnumeration("WebCore.IndexedDB.BackingStore.OpenStatus", IDBLevelDBBackingStoreOpenCleanupReopenSuccess, IDBLevelDBBackingStoreOpenMax);
+        LOG_ERROR("IndexedDB backing store cleanup succeeded, reopening");
+        db = levelDBFactory->openLevelDB(path, comparator.get());
+        if (!db) {
+            LOG_ERROR("IndexedDB backing store reopen after recovery failed");
+            HistogramSupport::histogramEnumeration("WebCore.IndexedDB.BackingStore.OpenStatus", IDBLevelDBBackingStoreOpenCleanupReopenFailed, IDBLevelDBBackingStoreOpenMax);
+            return PassRefPtr<IDBBackingStore>();
         }
+        HistogramSupport::histogramEnumeration("WebCore.IndexedDB.BackingStore.OpenStatus", IDBLevelDBBackingStoreOpenCleanupReopenSuccess, IDBLevelDBBackingStoreOpenMax);
     }
 
     if (!db) {
@@ -445,12 +444,37 @@ PassRefPtr<IDBBackingStore> IDBBackingStore::open(SecurityOrigin* securityOrigin
         return PassRefPtr<IDBBackingStore>();
     }
 
-    // FIXME: Handle comparator name changes.
+    return create(fileIdentifier, db.release(), comparator.release());
+}
+
+PassRefPtr<IDBBackingStore> IDBBackingStore::openInMemory(SecurityOrigin* securityOrigin, const String& identifier)
+{
+    DefaultLevelDBFactory levelDBFactory;
+    return IDBBackingStore::openInMemory(securityOrigin, identifier, &levelDBFactory);
+}
+
+PassRefPtr<IDBBackingStore> IDBBackingStore::openInMemory(SecurityOrigin* securityOrigin, const String& identifier, LevelDBFactory* levelDBFactory)
+{
+    IDB_TRACE("IDBBackingStore::openInMemory");
 
-    RefPtr<IDBBackingStore> backingStore(adoptRef(new IDBBackingStore(fileIdentifier, db.release())));
-    backingStore->m_comparator = comparator.release();
+    OwnPtr<LevelDBComparator> comparator = adoptPtr(new Comparator());
+    OwnPtr<LevelDBDatabase> db = LevelDBDatabase::openInMemory(comparator.get());
+    if (!db) {
+        LOG_ERROR("LevelDBDatabase::openInMemory failed.");
+        HistogramSupport::histogramEnumeration("WebCore.IndexedDB.BackingStore.OpenStatus", IDBLevelDBBackingStoreOpenMemoryFailed, IDBLevelDBBackingStoreOpenMax);
+        return PassRefPtr<IDBBackingStore>();
+    }
+    HistogramSupport::histogramEnumeration("WebCore.IndexedDB.BackingStore.OpenStatus", IDBLevelDBBackingStoreOpenMemorySuccess, IDBLevelDBBackingStoreOpenMax);
+
+    return create(identifier, db.release(), comparator.release());
+}
+
+PassRefPtr<IDBBackingStore> IDBBackingStore::create(const String& identifier, PassOwnPtr<LevelDBDatabase> db, PassOwnPtr<LevelDBComparator> comparator)
+{
+    // FIXME: Handle comparator name changes.
+    RefPtr<IDBBackingStore> backingStore(adoptRef(new IDBBackingStore(identifier, db, comparator)));
 
-    if (!setUpMetadata(backingStore->m_db.get(), fileIdentifier))
+    if (!setUpMetadata(backingStore->m_db.get(), identifier))
         return PassRefPtr<IDBBackingStore>();
 
     return backingStore.release();
index a23a7bc..8409e08 100644 (file)
@@ -59,6 +59,8 @@ public:
     virtual ~IDBBackingStore();
     static PassRefPtr<IDBBackingStore> open(SecurityOrigin*, const String& pathBase, const String& fileIdentifier);
     static PassRefPtr<IDBBackingStore> open(SecurityOrigin*, const String& pathBase, const String& fileIdentifier, LevelDBFactory*);
+    static PassRefPtr<IDBBackingStore> openInMemory(SecurityOrigin*, const String& identifier);
+    static PassRefPtr<IDBBackingStore> openInMemory(SecurityOrigin*, const String& identifier, LevelDBFactory*);
     WeakPtr<IDBBackingStore> createWeakPtr() { return m_weakFactory.createWeakPtr(); }
 
     virtual Vector<String> getDatabaseNames();
@@ -176,12 +178,14 @@ public:
     };
 
 protected:
-    IDBBackingStore(const String& identifier, PassOwnPtr<LevelDBDatabase>);
+    IDBBackingStore(const String& identifier, PassOwnPtr<LevelDBDatabase>, PassOwnPtr<LevelDBComparator>);
 
     // Should only used for mocking.
     IDBBackingStore();
 
 private:
+    static PassRefPtr<IDBBackingStore> create(const String& identifier, PassOwnPtr<LevelDBDatabase>, PassOwnPtr<LevelDBComparator>);
+
     bool findKeyInIndex(IDBBackingStore::Transaction*, int64_t databaseId, int64_t objectStoreId, int64_t indexId, const IDBKey&, Vector<char>& foundEncodedPrimaryKey, bool& found);
     bool getIndexes(int64_t databaseId, int64_t objectStoreId, IDBObjectStoreMetadata::IndexMap*) WARN_UNUSED_RETURN;
 
index 57807ca..84efc0d 100644 (file)
@@ -131,17 +131,28 @@ void IDBFactoryBackendImpl::deleteDatabase(const String& name, PassRefPtr<IDBCal
 PassRefPtr<IDBBackingStore> IDBFactoryBackendImpl::openBackingStore(PassRefPtr<SecurityOrigin> securityOrigin, const String& dataDirectory)
 {
     const String fileIdentifier = computeFileIdentifier(securityOrigin.get());
+    const bool openInMemory = dataDirectory.isEmpty();
 
     IDBBackingStoreMap::iterator it2 = m_backingStoreMap.find(fileIdentifier);
-    if (it2 != m_backingStoreMap.end()) {
-        if (it2->value.get())
-            return it2->value.get();
-    }
+    if (it2 != m_backingStoreMap.end() && it2->value.get())
+        return it2->value.get();
+
+    RefPtr<IDBBackingStore> backingStore;
+    if (openInMemory)
+        backingStore = IDBBackingStore::openInMemory(securityOrigin.get(), fileIdentifier);
+    else
+        backingStore = IDBBackingStore::open(securityOrigin.get(), dataDirectory, fileIdentifier);
 
-    RefPtr<IDBBackingStore> backingStore = IDBBackingStore::open(securityOrigin.get(), dataDirectory, fileIdentifier);
     if (backingStore) {
         cleanWeakMap(m_backingStoreMap);
         m_backingStoreMap.set(fileIdentifier, backingStore->createWeakPtr());
+        // If an in-memory database, bind lifetime to this factory instance.
+        if (openInMemory)
+            m_sessionOnlyBackingStores.add(backingStore);
+
+        // All backing stores associated with this factory should be of the same type.
+        ASSERT(m_sessionOnlyBackingStores.isEmpty() || openInMemory);
+
         return backingStore.release();
     }
 
index 8c8a86a..6be174f 100644 (file)
@@ -33,6 +33,7 @@
 #include "IDBFactoryBackendInterface.h"
 #include "SecurityOrigin.h"
 #include <wtf/HashMap.h>
+#include <wtf/HashSet.h>
 #include <wtf/RefCounted.h>
 #include <wtf/WeakPtr.h>
 #include <wtf/text/StringHash.h>
@@ -73,6 +74,8 @@ private:
     typedef HashMap<String, WeakPtr<IDBBackingStore> > IDBBackingStoreMap;
     IDBBackingStoreMap m_backingStoreMap;
 
+    HashSet<RefPtr<IDBBackingStore> > m_sessionOnlyBackingStores;
+
     // Only one instance of the factory should exist at any given time.
     static IDBFactoryBackendImpl* idbFactoryBackendImpl;
 };
index fb61ec3..14670af 100644 (file)
@@ -1,3 +1,14 @@
+2013-03-29  Joshua Bell  <jsbell@chromium.org>
+
+        IndexedDB: Bind lifetime of in-memory backing stores to IDBFactory backend
+        https://bugs.webkit.org/show_bug.cgi?id=110820
+
+        Reviewed by Tony Chang.
+
+        * tests/IDBBackingStoreTest.cpp:
+        (WebCore::IDBBackingStoreTest::SetUp): Use openInMemory rather than empty path.
+        (WebCore::TEST): Added IDBFactoryBackendTest.MemoryBackingStoreLifetime to verify refs.
+
 2013-03-28  James Robinson  <jamesr@chromium.org>
 
         [chromium] Remove unused WebRegularExpression API
index 0197a15..57b6be2 100644 (file)
@@ -29,6 +29,7 @@
 
 #include "IDBFactoryBackendImpl.h"
 #include "IDBLevelDBCoding.h"
+#include "SecurityOrigin.h"
 #include "SharedBuffer.h"
 
 #include <gtest/gtest.h>
@@ -47,10 +48,8 @@ public:
     void SetUp()
     {
         SecurityOrigin* securityOrigin = 0;
-        // Empty pathBase means an in-memory database.
-        String pathBase;
         String fileIdentifier;
-        m_backingStore = IDBBackingStore::open(securityOrigin, pathBase, fileIdentifier);
+        m_backingStore = IDBBackingStore::openInMemory(securityOrigin, fileIdentifier);
 
         // useful keys and values during tests
         const char rawValue1[] = "value1";
@@ -290,12 +289,33 @@ TEST(IDBFactoryBackendTest, BackingStoreLifetime)
     EXPECT_TRUE(diskStore1->hasOneRef());
 
     RefPtr<IDBBackingStore> diskStore2 = factory->testOpenBackingStore(origin1, path);
-    EXPECT_EQ(diskStore2.get(), diskStore1.get());
-    EXPECT_EQ(diskStore2->refCount(), 2);
+    EXPECT_EQ(diskStore1.get(), diskStore2.get());
+    EXPECT_EQ(2, diskStore2->refCount());
 
     RefPtr<IDBBackingStore> diskStore3 = factory->testOpenBackingStore(origin2, path);
     EXPECT_TRUE(diskStore3->hasOneRef());
-    EXPECT_EQ(diskStore1->refCount(), 2);
+    EXPECT_EQ(2, diskStore1->refCount());
+}
+
+TEST(IDBFactoryBackendTest, MemoryBackingStoreLifetime)
+{
+    RefPtr<SecurityOrigin> origin1 = SecurityOrigin::create("http", "localhost", 81);
+    RefPtr<SecurityOrigin> origin2 = SecurityOrigin::create("http", "localhost", 82);
+
+    RefPtr<MockIDBFactoryBackend> factory = MockIDBFactoryBackend::create();
+    RefPtr<IDBBackingStore> memStore1 = factory->testOpenBackingStore(origin1, String());
+    EXPECT_EQ(2, memStore1->refCount());
+    RefPtr<IDBBackingStore> memStore2 = factory->testOpenBackingStore(origin1, String());
+    EXPECT_EQ(memStore1.get(), memStore2.get());
+    EXPECT_EQ(3, memStore2->refCount());
+
+    RefPtr<IDBBackingStore> memStore3 = factory->testOpenBackingStore(origin2, String());
+    EXPECT_EQ(2, memStore3->refCount());
+    EXPECT_EQ(3, memStore1->refCount());
+
+    factory.clear();
+    EXPECT_EQ(2, memStore1->refCount());
+    EXPECT_EQ(1, memStore3->refCount());
 }
 
 } // namespace