Diff
Modified: trunk/Source/WebCore/ChangeLog (145237 => 145238)
--- trunk/Source/WebCore/ChangeLog 2013-03-08 17:42:13 UTC (rev 145237)
+++ trunk/Source/WebCore/ChangeLog 2013-03-08 18:06:23 UTC (rev 145238)
@@ -1,3 +1,34 @@
+2013-03-08 Joshua Bell <jsb...@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<T>>. 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-08 John Mellor <joh...@chromium.org>
@media queries do not take zooming into account
Modified: trunk/Source/WebCore/Modules/indexeddb/IDBBackingStore.cpp (145237 => 145238)
--- trunk/Source/WebCore/Modules/indexeddb/IDBBackingStore.cpp 2013-03-08 17:42:13 UTC (rev 145237)
+++ trunk/Source/WebCore/Modules/indexeddb/IDBBackingStore.cpp 2013-03-08 18:06:23 UTC (rev 145238)
@@ -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 @@
}
};
-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 @@
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 @@
// 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))
Modified: trunk/Source/WebCore/Modules/indexeddb/IDBBackingStore.h (145237 => 145238)
--- trunk/Source/WebCore/Modules/indexeddb/IDBBackingStore.h 2013-03-08 17:42:13 UTC (rev 145237)
+++ trunk/Source/WebCore/Modules/indexeddb/IDBBackingStore.h 2013-03-08 18:06:23 UTC (rev 145238)
@@ -34,13 +34,13 @@
#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 @@
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 @@
};
protected:
- IDBBackingStore(const String& identifier, IDBFactoryBackendImpl*, PassOwnPtr<LevelDBDatabase>);
+ IDBBackingStore(const String& identifier, PassOwnPtr<LevelDBDatabase>);
// Should only used for mocking.
IDBBackingStore();
@@ -180,10 +181,10 @@
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
Modified: trunk/Source/WebCore/Modules/indexeddb/IDBFactoryBackendImpl.cpp (145237 => 145238)
--- trunk/Source/WebCore/Modules/indexeddb/IDBFactoryBackendImpl.cpp 2013-03-08 17:42:13 UTC (rev 145237)
+++ trunk/Source/WebCore/Modules/indexeddb/IDBFactoryBackendImpl.cpp 2013-03-08 18:06:23 UTC (rev 145238)
@@ -41,6 +41,20 @@
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 @@
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 @@
{
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;
}
Modified: trunk/Source/WebCore/Modules/indexeddb/IDBFactoryBackendImpl.h (145237 => 145238)
--- trunk/Source/WebCore/Modules/indexeddb/IDBFactoryBackendImpl.h 2013-03-08 17:42:13 UTC (rev 145237)
+++ trunk/Source/WebCore/Modules/indexeddb/IDBFactoryBackendImpl.h 2013-03-08 18:06:23 UTC (rev 145238)
@@ -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 @@
// 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 @@
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.
Modified: trunk/Source/WebKit/chromium/ChangeLog (145237 => 145238)
--- trunk/Source/WebKit/chromium/ChangeLog 2013-03-08 17:42:13 UTC (rev 145237)
+++ trunk/Source/WebKit/chromium/ChangeLog 2013-03-08 18:06:23 UTC (rev 145238)
@@ -1,3 +1,20 @@
+2013-03-08 Joshua Bell <jsb...@chromium.org>
+
+ IndexedDB: Use WeakPtr for Factory-to-BackingStore reference
+ https://bugs.webkit.org/show_bug.cgi?id=111459
+
+ Reviewed by Adam Barth.
+
+ Add new test that verifies refcounts.
+
+ * WebKit.gyp: Don't include files depending on webkit_support.
+ * tests/IDBBackingStoreTest.cpp:
+ (WebCore::IDBBackingStoreTest::SetUp): No need for dummy factory.
+ (MockIDBFactoryBackend): Allow access to protected openBackingStore method.
+ (WebCore::TEST): Add new test that verifies refcounts.
+ * tests/IDBCleanupOnIOErrorTest.cpp:
+ (WebCore::TEST): No need for dummy factory.
+
2013-03-07 Keishi Hattori <kei...@webkit.org>
Update calendar picker UI
Modified: trunk/Source/WebKit/chromium/WebKit.gyp (145237 => 145238)
--- trunk/Source/WebKit/chromium/WebKit.gyp 2013-03-08 17:42:13 UTC (rev 145237)
+++ trunk/Source/WebKit/chromium/WebKit.gyp 2013-03-08 18:06:23 UTC (rev 145238)
@@ -653,6 +653,7 @@
# We should not include files depending on webkit_support.
# These tests depend on webkit_support and
# functions defined only in !WEBKIT_IMPLEMENTATION.
+ 'tests/IDBBackingStoreTest.cpp',
'tests/IDBCleanupOnIOErrorTest.cpp',
'tests/LevelDBTest.cpp',
],
Modified: trunk/Source/WebKit/chromium/tests/IDBBackingStoreTest.cpp (145237 => 145238)
--- trunk/Source/WebKit/chromium/tests/IDBBackingStoreTest.cpp 2013-03-08 17:42:13 UTC (rev 145237)
+++ trunk/Source/WebKit/chromium/tests/IDBBackingStoreTest.cpp 2013-03-08 18:06:23 UTC (rev 145238)
@@ -27,9 +27,11 @@
#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 @@
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 @@
}
protected:
- IDBFactoryBackendImpl* m_factory;
RefPtr<IDBBackingStore> m_backingStore;
// Sample keys and values that are consistent.
@@ -183,6 +183,42 @@
}
}
+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)
Modified: trunk/Source/WebKit/chromium/tests/IDBCleanupOnIOErrorTest.cpp (145237 => 145238)
--- trunk/Source/WebKit/chromium/tests/IDBCleanupOnIOErrorTest.cpp 2013-03-08 17:42:13 UTC (rev 145237)
+++ trunk/Source/WebKit/chromium/tests/IDBCleanupOnIOErrorTest.cpp 2013-03-08 18:06:23 UTC (rev 145238)
@@ -77,9 +77,8 @@
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
