IndexedDB: Use WeakPtr for Factory-to-BackingStore reference
authorjsbell@chromium.org <jsbell@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 8 Mar 2013 01:42:17 +0000 (01:42 +0000)
committerjsbell@chromium.org <jsbell@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 8 Mar 2013 01:42:17 +0000 (01:42 +0000)
https://bugs.webkit.org/show_bug.cgi?id=111459

Reviewed by Adam Barth.

Source/WebCore:

IDBFactoryBackendImpl maintains a map of backing stores - if another database in the same
origin is opened, the backing store instance must be re-used). This was a map to raw
pointers so that the backing store can be collected when all database references are
dropped. The map was maintained manually by passing the factory to the IDBBackingStore which
would add/remove itself on creation/destruction.

Replace this with a HashMap<WeakPtr<>>. This simplifies the plumbing; map entries
"leak" but are purged on subsequent opens.

Added webkit_unit_test (Chromium port) to verify refcounts.

* Modules/indexeddb/IDBBackingStore.cpp:
(WebCore::IDBBackingStore::IDBBackingStore): No need to notify factory of lifetime.
(WebCore::IDBBackingStore::~IDBBackingStore): Ditto.
(WebCore::IDBBackingStore::open): Ditto.
* Modules/indexeddb/IDBBackingStore.h: No reference to the factory, but...
(WebCore::IDBBackingStore::createWeakPtr): Do need to expose weak pointers for the factory to hold.
* Modules/indexeddb/IDBFactoryBackendImpl.cpp:
(WebCore::cleanWeakMap): Helper function to scrub a HashMap<WeakPtr<T>> of empty pointers.
May move to WTF when we've learned how general it is, or come up with a dedicated WeakPtrHashMap type.
(WebCore::IDBFactoryBackendImpl::openBackingStore): WeakPtr fu.
* Modules/indexeddb/IDBFactoryBackendImpl.h:
(IDBFactoryBackendImpl): Remove plumbing methods.

Source/WebKit/chromium:

Added tests to verify refcounts on backing stores, update method signatures.

* tests/IDBBackingStoreTest.cpp:
(WebCore::IDBBackingStoreTest::SetUp): No dummy factory needed.
(MockIDBFactoryBackend): Expose protected method to tests.
(WebCore::TEST): Added BackingStoreLifetime test.
* tests/IDBCleanupOnIOErrorTest.cpp: No dummy factory needed.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@145166 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
Source/WebKit/chromium/tests/IDBCleanupOnIOErrorTest.cpp

index 531b824..7b8bd92 100644 (file)
@@ -1,3 +1,34 @@
+2013-03-07  Joshua Bell  <jsbell@chromium.org>
+
+        IndexedDB: Use WeakPtr for Factory-to-BackingStore reference
+        https://bugs.webkit.org/show_bug.cgi?id=111459
+
+        Reviewed by Adam Barth.
+
+        IDBFactoryBackendImpl maintains a map of backing stores - if another database in the same
+        origin is opened, the backing store instance must be re-used). This was a map to raw
+        pointers so that the backing store can be collected when all database references are
+        dropped. The map was maintained manually by passing the factory to the IDBBackingStore which
+        would add/remove itself on creation/destruction.
+
+        Replace this with a HashMap<WeakPtr<>>. This simplifies the plumbing; map entries
+        "leak" but are purged on subsequent opens.
+
+        Added webkit_unit_test (Chromium port) to verify refcounts.
+
+        * Modules/indexeddb/IDBBackingStore.cpp:
+        (WebCore::IDBBackingStore::IDBBackingStore): No need to notify factory of lifetime.
+        (WebCore::IDBBackingStore::~IDBBackingStore): Ditto.
+        (WebCore::IDBBackingStore::open): Ditto.
+        * Modules/indexeddb/IDBBackingStore.h: No reference to the factory, but...
+        (WebCore::IDBBackingStore::createWeakPtr): Do need to expose weak pointers for the factory to hold.
+        * Modules/indexeddb/IDBFactoryBackendImpl.cpp:
+        (WebCore::cleanWeakMap): Helper function to scrub a HashMap<WeakPtr<T>> of empty pointers.
+        May move to WTF when we've learned how general it is, or come up with a dedicated WeakPtrHashMap type.
+        (WebCore::IDBFactoryBackendImpl::openBackingStore): WeakPtr fu.
+        * Modules/indexeddb/IDBFactoryBackendImpl.h:
+        (IDBFactoryBackendImpl): Remove plumbing methods.
+
 2013-03-07  Otto Derek Cheung  <otcheung@rim.com>
 
         [BlackBerry] RefCounting ParsedCookie to avoid SegFaults
index e37d7a6..0abcfed 100644 (file)
@@ -30,7 +30,6 @@
 
 #include "FileSystem.h"
 #include "HistogramSupport.h"
-#include "IDBFactoryBackendImpl.h"
 #include "IDBKey.h"
 #include "IDBKeyPath.h"
 #include "IDBKeyRange.h"
@@ -44,9 +43,6 @@
 #include "LevelDBTransaction.h"
 #include "SecurityOrigin.h"
 #include "SharedBuffer.h"
-#if PLATFORM(CHROMIUM)
-#include <public/Platform.h>
-#endif
 #include <wtf/Assertions.h>
 
 namespace WebCore {
@@ -339,28 +335,20 @@ public:
     }
 };
 
-IDBBackingStore::IDBBackingStore(const String& identifier, IDBFactoryBackendImpl* factory, PassOwnPtr<LevelDBDatabase> db)
+IDBBackingStore::IDBBackingStore(const String& identifier, PassOwnPtr<LevelDBDatabase> db)
     : m_identifier(identifier)
-    , m_factory(factory)
     , m_db(db)
+    , m_weakFactory(this)
 {
-#if PLATFORM(CHROMIUM)
-    ASSERT(m_factory || WebKit::Platform::current()->unitTestSupport());
-#endif
-    if (m_factory)
-        m_factory->addIDBBackingStore(identifier, this);
 }
 
 IDBBackingStore::IDBBackingStore()
+    : m_weakFactory(this)
 {
 }
 
 IDBBackingStore::~IDBBackingStore()
 {
-    // Only null in tests.
-    if (m_factory)
-        m_factory->removeIDBBackingStore(m_identifier);
-
     // m_db's destructor uses m_comparator. The order of destruction is important.
     m_db.clear();
     m_comparator.clear();
@@ -380,13 +368,13 @@ enum IDBLevelDBBackingStoreOpenResult {
     IDBLevelDBBackingStoreOpenMax,
 };
 
-PassRefPtr<IDBBackingStore> IDBBackingStore::open(SecurityOrigin* securityOrigin, const String& pathBaseArg, const String& fileIdentifier, IDBFactoryBackendImpl* factory)
+PassRefPtr<IDBBackingStore> IDBBackingStore::open(SecurityOrigin* securityOrigin, const String& pathBaseArg, const String& fileIdentifier)
 {
     DefaultLevelDBFactory levelDBFactory;
-    return IDBBackingStore::open(securityOrigin, pathBaseArg, fileIdentifier, factory, &levelDBFactory);
+    return IDBBackingStore::open(securityOrigin, pathBaseArg, fileIdentifier, &levelDBFactory);
 }
 
-PassRefPtr<IDBBackingStore> IDBBackingStore::open(SecurityOrigin* securityOrigin, const String& pathBaseArg, const String& fileIdentifier, IDBFactoryBackendImpl* factory, LevelDBFactory* levelDBFactory)
+PassRefPtr<IDBBackingStore> IDBBackingStore::open(SecurityOrigin* securityOrigin, const String& pathBaseArg, const String& fileIdentifier, LevelDBFactory* levelDBFactory)
 {
     IDB_TRACE("IDBBackingStore::open");
     String pathBase = pathBaseArg;
@@ -456,7 +444,7 @@ PassRefPtr<IDBBackingStore> IDBBackingStore::open(SecurityOrigin* securityOrigin
 
     // FIXME: Handle comparator name changes.
 
-    RefPtr<IDBBackingStore> backingStore(adoptRef(new IDBBackingStore(fileIdentifier, factory, db.release())));
+    RefPtr<IDBBackingStore> backingStore(adoptRef(new IDBBackingStore(fileIdentifier, db.release())));
     backingStore->m_comparator = comparator.release();
 
     if (!setUpMetadata(backingStore->m_db.get(), fileIdentifier))
index fcfe2da..e8ee6b6 100644 (file)
 #include "LevelDBTransaction.h"
 #include <wtf/OwnPtr.h>
 #include <wtf/RefCounted.h>
+#include <wtf/WeakPtr.h>
 
 namespace WebCore {
 
 class LevelDBComparator;
 class LevelDBDatabase;
 class LevelDBTransaction;
-class IDBFactoryBackendImpl;
 class IDBKey;
 class IDBKeyRange;
 class SecurityOrigin;
@@ -57,8 +57,9 @@ public:
     class Transaction;
 
     virtual ~IDBBackingStore();
-    static PassRefPtr<IDBBackingStore> open(SecurityOrigin*, const String& pathBase, const String& fileIdentifier, IDBFactoryBackendImpl*);
-    static PassRefPtr<IDBBackingStore> open(SecurityOrigin*, const String& pathBase, const String& fileIdentifier, IDBFactoryBackendImpl*, LevelDBFactory*);
+    static PassRefPtr<IDBBackingStore> open(SecurityOrigin*, const String& pathBase, const String& fileIdentifier);
+    static PassRefPtr<IDBBackingStore> open(SecurityOrigin*, const String& pathBase, const String& fileIdentifier, LevelDBFactory*);
+    WeakPtr<IDBBackingStore> createWeakPtr() { return m_weakFactory.createWeakPtr(); }
 
     virtual Vector<String> getDatabaseNames();
     virtual bool getIDBDatabaseMetaData(const String& name, IDBDatabaseMetadata*, bool& success) WARN_UNUSED_RETURN;
@@ -170,7 +171,7 @@ public:
     };
 
 protected:
-    IDBBackingStore(const String& identifier, IDBFactoryBackendImpl*, PassOwnPtr<LevelDBDatabase>);
+    IDBBackingStore(const String& identifier, PassOwnPtr<LevelDBDatabase>);
 
     // Should only used for mocking.
     IDBBackingStore();
@@ -180,10 +181,10 @@ private:
     void getIndexes(int64_t databaseId, int64_t objectStoreId, IDBObjectStoreMetadata::IndexMap*);
 
     String m_identifier;
-    RefPtr<IDBFactoryBackendImpl> m_factory;
+
     OwnPtr<LevelDBDatabase> m_db;
     OwnPtr<LevelDBComparator> m_comparator;
-
+    WeakPtrFactory<IDBBackingStore> m_weakFactory;
 };
 
 } // namespace WebCore
index 23c616e..4191750 100644 (file)
 
 namespace WebCore {
 
+template<typename K, typename M>
+static void cleanWeakMap(HashMap<K, WeakPtr<M> >& map)
+{
+    HashMap<K, WeakPtr<M> > other;
+    other.swap(map);
+
+    typename HashMap<K, WeakPtr<M> >::const_iterator iter = other.begin();
+    while (iter != other.end()) {
+        if (iter->value.get())
+            map.set(iter->key, iter->value);
+        ++iter;
+    }
+}
+
 static String computeFileIdentifier(SecurityOrigin* securityOrigin)
 {
     static const char levelDBFileSuffix[] = "@1";
@@ -66,18 +80,6 @@ void IDBFactoryBackendImpl::removeIDBDatabaseBackend(const String& uniqueIdentif
     m_databaseBackendMap.remove(uniqueIdentifier);
 }
 
-void IDBFactoryBackendImpl::addIDBBackingStore(const String& fileIdentifier, IDBBackingStore* backingStore)
-{
-    ASSERT(!m_backingStoreMap.contains(fileIdentifier));
-    m_backingStoreMap.set(fileIdentifier, backingStore);
-}
-
-void IDBFactoryBackendImpl::removeIDBBackingStore(const String& fileIdentifier)
-{
-    ASSERT(m_backingStoreMap.contains(fileIdentifier));
-    m_backingStoreMap.remove(fileIdentifier);
-}
-
 void IDBFactoryBackendImpl::getDatabaseNames(PassRefPtr<IDBCallbacks> callbacks, PassRefPtr<SecurityOrigin> securityOrigin, ScriptExecutionContext*, const String& dataDirectory)
 {
     RefPtr<IDBBackingStore> backingStore = openBackingStore(securityOrigin, dataDirectory);
@@ -127,16 +129,18 @@ PassRefPtr<IDBBackingStore> IDBFactoryBackendImpl::openBackingStore(PassRefPtr<S
 {
     const String fileIdentifier = computeFileIdentifier(securityOrigin.get());
 
-    RefPtr<IDBBackingStore> backingStore;
     IDBBackingStoreMap::iterator it2 = m_backingStoreMap.find(fileIdentifier);
-    if (it2 != m_backingStoreMap.end())
-        backingStore = it2->value;
-    else {
-        backingStore = IDBBackingStore::open(securityOrigin.get(), dataDirectory, fileIdentifier, this);
+    if (it2 != m_backingStoreMap.end()) {
+        if (it2->value.get())
+            return it2->value.get();
     }
 
-    if (backingStore)
+    RefPtr<IDBBackingStore> backingStore = IDBBackingStore::open(securityOrigin.get(), dataDirectory, fileIdentifier);
+    if (backingStore) {
+        cleanWeakMap(m_backingStoreMap);
+        m_backingStoreMap.set(fileIdentifier, backingStore->createWeakPtr());
         return backingStore.release();
+    }
 
     return 0;
 }
index 24f73ae..8c8a86a 100644 (file)
@@ -33,6 +33,8 @@
 #include "IDBFactoryBackendInterface.h"
 #include "SecurityOrigin.h"
 #include <wtf/HashMap.h>
+#include <wtf/RefCounted.h>
+#include <wtf/WeakPtr.h>
 #include <wtf/text/StringHash.h>
 
 #if ENABLE(INDEXED_DATABASE)
@@ -54,8 +56,6 @@ public:
 
     // Notifications from weak pointers.
     virtual void removeIDBDatabaseBackend(const String& uniqueIdentifier);
-    void addIDBBackingStore(const String& fileIdentifier, IDBBackingStore*);
-    virtual void removeIDBBackingStore(const String& fileIdentifier);
 
     virtual void getDatabaseNames(PassRefPtr<IDBCallbacks>, PassRefPtr<SecurityOrigin>, ScriptExecutionContext*, const String& dataDir);
     virtual void open(const String& name, int64_t version, int64_t transactionId, PassRefPtr<IDBCallbacks>, PassRefPtr<IDBDatabaseCallbacks>, PassRefPtr<SecurityOrigin>, ScriptExecutionContext*, const String& dataDir);
@@ -70,7 +70,7 @@ private:
     typedef HashMap<String, RefPtr<IDBDatabaseBackendImpl> > IDBDatabaseBackendMap;
     IDBDatabaseBackendMap m_databaseBackendMap;
 
-    typedef HashMap<String, IDBBackingStore*> IDBBackingStoreMap;
+    typedef HashMap<String, WeakPtr<IDBBackingStore> > IDBBackingStoreMap;
     IDBBackingStoreMap m_backingStoreMap;
 
     // Only one instance of the factory should exist at any given time.
index 2482c57..6e16b7f 100644 (file)
@@ -1,3 +1,18 @@
+2013-03-07  Joshua Bell  <jsbell@chromium.org>
+
+        IndexedDB: Use WeakPtr for Factory-to-BackingStore reference
+        https://bugs.webkit.org/show_bug.cgi?id=111459
+
+        Reviewed by Adam Barth.
+
+        Added tests to verify refcounts on backing stores, update method signatures.
+
+        * tests/IDBBackingStoreTest.cpp:
+        (WebCore::IDBBackingStoreTest::SetUp): No dummy factory needed.
+        (MockIDBFactoryBackend): Expose protected method to tests.
+        (WebCore::TEST): Added BackingStoreLifetime test.
+        * tests/IDBCleanupOnIOErrorTest.cpp: No dummy factory needed.
+
 2013-03-06  James Robinson  <jamesr@chromium.org>
 
         [chromium] Stop using WebTransformationMatrix on WebLayer
index aa4cdc9..5302e12 100644 (file)
 
 #include "IDBBackingStore.h"
 
+#include "IDBFactoryBackendImpl.h"
 #include "SharedBuffer.h"
 
 #include <gtest/gtest.h>
+#include <webkit/support/webkit_support.h>
 
 #if ENABLE(INDEXED_DATABASE)
 
@@ -43,11 +45,10 @@ public:
     void SetUp()
     {
         SecurityOrigin* securityOrigin = 0;
-        m_factory = 0;
         // Empty pathBase means an in-memory database.
         String pathBase;
         String fileIdentifier;
-        m_backingStore = IDBBackingStore::open(securityOrigin, pathBase, fileIdentifier, m_factory);
+        m_backingStore = IDBBackingStore::open(securityOrigin, pathBase, fileIdentifier);
 
         // useful keys and values during tests
         const char rawValue1[] = "value1";
@@ -59,7 +60,6 @@ public:
     }
 
 protected:
-    IDBFactoryBackendImpl* m_factory;
     RefPtr<IDBBackingStore> m_backingStore;
 
     // Sample keys and values that are consistent.
@@ -183,6 +183,42 @@ TEST_F(IDBBackingStoreTest, CreateDatabase)
     }
 }
 
+class MockIDBFactoryBackend : public IDBFactoryBackendImpl {
+public:
+    static PassRefPtr<MockIDBFactoryBackend> create()
+    {
+        return adoptRef(new MockIDBFactoryBackend());
+    }
+
+    PassRefPtr<IDBBackingStore> testOpenBackingStore(PassRefPtr<SecurityOrigin> origin, const String& dataDirectory)
+    {
+        return openBackingStore(origin, dataDirectory);
+    }
+};
+
+TEST(IDBFactoryBackendTest, BackingStoreLifetime)
+{
+    RefPtr<SecurityOrigin> origin1 = SecurityOrigin::create("http", "localhost", 81);
+    RefPtr<SecurityOrigin> origin2 = SecurityOrigin::create("http", "localhost", 82);
+
+    RefPtr<MockIDBFactoryBackend> factory = MockIDBFactoryBackend::create();
+
+    OwnPtr<webkit_support::ScopedTempDirectory> tempDirectory = adoptPtr(webkit_support::CreateScopedTempDirectory());
+    tempDirectory->CreateUniqueTempDir();
+    const String path = String::fromUTF8(tempDirectory->path().c_str());
+
+    RefPtr<IDBBackingStore> diskStore1 = factory->testOpenBackingStore(origin1, path);
+    EXPECT_TRUE(diskStore1->hasOneRef());
+
+    RefPtr<IDBBackingStore> diskStore2 = factory->testOpenBackingStore(origin1, path);
+    EXPECT_EQ(diskStore2.get(), diskStore1.get());
+    EXPECT_EQ(diskStore2->refCount(), 2);
+
+    RefPtr<IDBBackingStore> diskStore3 = factory->testOpenBackingStore(origin2, path);
+    EXPECT_TRUE(diskStore3->hasOneRef());
+    EXPECT_EQ(diskStore1->refCount(), 2);
+}
+
 } // namespace
 
 #endif // ENABLE(INDEXED_DATABASE)
index 90dccd6..743e5c8 100644 (file)
@@ -77,9 +77,8 @@ TEST(IDBIOErrorTest, CleanUpTest)
     tempDirectory->CreateUniqueTempDir();
     const String path = String::fromUTF8(tempDirectory->path().c_str());
     String dummyFileIdentifier;
-    IDBFactoryBackendImpl* dummyIDBFactory = 0;
     MockLevelDBFactory mockLevelDBFactory;
-    RefPtr<IDBBackingStore> backingStore = IDBBackingStore::open(origin.get(), path, dummyFileIdentifier, dummyIDBFactory, &mockLevelDBFactory);
+    RefPtr<IDBBackingStore> backingStore = IDBBackingStore::open(origin.get(), path, dummyFileIdentifier, &mockLevelDBFactory);
 }
 
 } // namespace