Commit 914f2dd1 authored by jorlow@chromium.org's avatar jorlow@chromium.org
Browse files

2009-09-21 Jeremy Orlow <jorlow@chromium.org>

        Reviewed by Adam Barth.

        DOM Storage needs to be more careful about where "ThreadSafe" objects are destroyed.
        https://bugs.webkit.org/show_bug.cgi?id=29265

        DOM Storage needs to be more careful about where "ThreadSafe" objects are
        destroyed.  With the current code, there actually isn't a race condition, but
        it sure would be easy for someone to introduce one.  A bunch of
        ThreadSafeShared objects have RefPtrs to objects that are NOT ThreadSafeShared
        objects.  If it were possible any of these objects' destructors to be fired off
        the main thread, then the you'd have a race condition.  The code should be more
        clear and self-documenting about how things related to each other.

        Since the lifetime of a LocalStorageTask is bounded by the LocalStorageThread
        which is bounded by the StorageSyncManager, StorageAreaImpl, and
        StorageAreaSync, there's no reason for LocalStorageTask to store anything other
        than pointers.  By breaking this dependency, we can eliminate the risk.

        Note that we _could_ have LocalStorageThread's task queue just store
        LocalStorageTask*'s rather than RefPtr<LocalStorageTask>s but then we'd need to
        manually take care of deleting.  It'd probably also be possible to change
        LocalStorageThread around so that it needn't hold onto a reference of itself
        and have a more deterministic shutdown, but my initial attempts to do so
        failed, and I decided it wasn't worth changing.  The queue is killed before
        hand, so the thread is 100% impotent before the main thread continues anyway.

        The constructors and destructors of StorageSyncManager, StorageAreaImpl, and
        StorageAreaSync now have ASSERTs to verify they're running on the main thread. 
        I'm fairly positive that it'd be impossible to hit these asserts and the fact
        that these classes are no longer ThreadSafeShared should make it clear how
        they're meant to be used, but I think it's worth it to be extra sure.  Of
        course, ideally, we'd have such an assert every time a ref is incremented or
        decremented.

        Behavior should be unchanged and this is just an internal code cleanup, so no
        new tests.

        * storage/LocalStorageTask.cpp:
        (WebCore::LocalStorageTask::LocalStorageTask):
        (WebCore::LocalStorageTask::performTask):
        * storage/LocalStorageTask.h:
        (WebCore::LocalStorageTask::createImport):
        (WebCore::LocalStorageTask::createSync):
        (WebCore::LocalStorageTask::createTerminate):
        * storage/LocalStorageThread.cpp:
        (WebCore::LocalStorageThread::scheduleImport):
        (WebCore::LocalStorageThread::scheduleSync):
        * storage/LocalStorageThread.h:
        * storage/StorageArea.h:
        * storage/StorageAreaImpl.cpp:
        (WebCore::StorageAreaImpl::~StorageAreaImpl):
        (WebCore::StorageAreaImpl::StorageAreaImpl):
        * storage/StorageAreaSync.cpp:
        (WebCore::StorageAreaSync::StorageAreaSync):
        (WebCore::StorageAreaSync::~StorageAreaSync):
        * storage/StorageSyncManager.cpp:
        (WebCore::StorageSyncManager::StorageSyncManager):
        (WebCore::StorageSyncManager::~StorageSyncManager):
        (WebCore::StorageSyncManager::scheduleImport):
        (WebCore::StorageSyncManager::scheduleSync):
        * storage/StorageSyncManager.h:


git-svn-id: http://svn.webkit.org/repository/webkit/trunk@48939 268f45cc-cd09-0410-ab3c-d52691b4dbfc
parent 834bfad2
2009-09-21 Jeremy Orlow <jorlow@chromium.org>
Reviewed by Adam Barth.
DOM Storage needs to be more careful about where "ThreadSafe" objects are destroyed.
https://bugs.webkit.org/show_bug.cgi?id=29265
DOM Storage needs to be more careful about where "ThreadSafe" objects are
destroyed. With the current code, there actually isn't a race condition, but
it sure would be easy for someone to introduce one. A bunch of
ThreadSafeShared objects have RefPtrs to objects that are NOT ThreadSafeShared
objects. If it were possible any of these objects' destructors to be fired off
the main thread, then the you'd have a race condition. The code should be more
clear and self-documenting about how things related to each other.
Since the lifetime of a LocalStorageTask is bounded by the LocalStorageThread
which is bounded by the StorageSyncManager, StorageAreaImpl, and
StorageAreaSync, there's no reason for LocalStorageTask to store anything other
than pointers. By breaking this dependency, we can eliminate the risk.
Note that we _could_ have LocalStorageThread's task queue just store
LocalStorageTask*'s rather than RefPtr<LocalStorageTask>s but then we'd need to
manually take care of deleting. It'd probably also be possible to change
LocalStorageThread around so that it needn't hold onto a reference of itself
and have a more deterministic shutdown, but my initial attempts to do so
failed, and I decided it wasn't worth changing. The queue is killed before
hand, so the thread is 100% impotent before the main thread continues anyway.
The constructors and destructors of StorageSyncManager, StorageAreaImpl, and
StorageAreaSync now have ASSERTs to verify they're running on the main thread.
I'm fairly positive that it'd be impossible to hit these asserts and the fact
that these classes are no longer ThreadSafeShared should make it clear how
they're meant to be used, but I think it's worth it to be extra sure. Of
course, ideally, we'd have such an assert every time a ref is incremented or
decremented.
Behavior should be unchanged and this is just an internal code cleanup, so no
new tests.
* storage/LocalStorageTask.cpp:
(WebCore::LocalStorageTask::LocalStorageTask):
(WebCore::LocalStorageTask::performTask):
* storage/LocalStorageTask.h:
(WebCore::LocalStorageTask::createImport):
(WebCore::LocalStorageTask::createSync):
(WebCore::LocalStorageTask::createTerminate):
* storage/LocalStorageThread.cpp:
(WebCore::LocalStorageThread::scheduleImport):
(WebCore::LocalStorageThread::scheduleSync):
* storage/LocalStorageThread.h:
* storage/StorageArea.h:
* storage/StorageAreaImpl.cpp:
(WebCore::StorageAreaImpl::~StorageAreaImpl):
(WebCore::StorageAreaImpl::StorageAreaImpl):
* storage/StorageAreaSync.cpp:
(WebCore::StorageAreaSync::StorageAreaSync):
(WebCore::StorageAreaSync::~StorageAreaSync):
* storage/StorageSyncManager.cpp:
(WebCore::StorageSyncManager::StorageSyncManager):
(WebCore::StorageSyncManager::~StorageSyncManager):
(WebCore::StorageSyncManager::scheduleImport):
(WebCore::StorageSyncManager::scheduleSync):
* storage/StorageSyncManager.h:
2009-09-28 Jeremy Orlow <jorlow@chromium.org>
 
Reviewed by Darin Fisher.
......@@ -33,16 +33,18 @@
namespace WebCore {
LocalStorageTask::LocalStorageTask(Type type, PassRefPtr<StorageAreaSync> area)
LocalStorageTask::LocalStorageTask(Type type, StorageAreaSync* area)
: m_type(type)
, m_area(area)
, m_thread(0)
{
ASSERT(m_area);
ASSERT(m_type == AreaImport || m_type == AreaSync);
}
LocalStorageTask::LocalStorageTask(Type type, PassRefPtr<LocalStorageThread> thread)
LocalStorageTask::LocalStorageTask(Type type, LocalStorageThread* thread)
: m_type(type)
, m_area(0)
, m_thread(thread)
{
ASSERT(m_thread);
......@@ -57,11 +59,9 @@ void LocalStorageTask::performTask()
{
switch (m_type) {
case AreaImport:
ASSERT(m_area);
m_area->performImport();
break;
case AreaSync:
ASSERT(m_area);
m_area->performSync();
break;
case TerminateThread:
......
......@@ -44,19 +44,19 @@ namespace WebCore {
~LocalStorageTask();
static PassRefPtr<LocalStorageTask> createImport(PassRefPtr<StorageAreaSync> area) { return adoptRef(new LocalStorageTask(AreaImport, area)); }
static PassRefPtr<LocalStorageTask> createSync(PassRefPtr<StorageAreaSync> area) { return adoptRef(new LocalStorageTask(AreaSync, area)); }
static PassRefPtr<LocalStorageTask> createTerminate(PassRefPtr<LocalStorageThread> thread) { return adoptRef(new LocalStorageTask(TerminateThread, thread)); }
static PassRefPtr<LocalStorageTask> createImport(StorageAreaSync* area) { return adoptRef(new LocalStorageTask(AreaImport, area)); }
static PassRefPtr<LocalStorageTask> createSync(StorageAreaSync* area) { return adoptRef(new LocalStorageTask(AreaSync, area)); }
static PassRefPtr<LocalStorageTask> createTerminate(LocalStorageThread* thread) { return adoptRef(new LocalStorageTask(TerminateThread, thread)); }
void performTask();
private:
LocalStorageTask(Type, PassRefPtr<StorageAreaSync>);
LocalStorageTask(Type, PassRefPtr<LocalStorageThread>);
LocalStorageTask(Type, StorageAreaSync*);
LocalStorageTask(Type, LocalStorageThread*);
Type m_type;
RefPtr<StorageAreaSync> m_area;
RefPtr<LocalStorageThread> m_thread;
StorageAreaSync* m_area;
LocalStorageThread* m_thread;
};
} // namespace WebCore
......
......@@ -86,13 +86,13 @@ void* LocalStorageThread::localStorageThread()
return 0;
}
void LocalStorageThread::scheduleImport(PassRefPtr<StorageAreaSync> area)
void LocalStorageThread::scheduleImport(StorageAreaSync* area)
{
ASSERT(!m_queue.killed() && m_threadID);
m_queue.append(LocalStorageTask::createImport(area));
}
void LocalStorageThread::scheduleSync(PassRefPtr<StorageAreaSync> area)
void LocalStorageThread::scheduleSync(StorageAreaSync* area)
{
ASSERT(!m_queue.killed() && m_threadID);
m_queue.append(LocalStorageTask::createSync(area));
......
......@@ -45,8 +45,8 @@ namespace WebCore {
bool start();
void scheduleImport(PassRefPtr<StorageAreaSync>);
void scheduleSync(PassRefPtr<StorageAreaSync>);
void scheduleImport(StorageAreaSync*);
void scheduleSync(StorageAreaSync*);
// Called from the main thread to synchronously shut down this thread
void terminate();
......
......@@ -31,7 +31,7 @@
#include "PlatformString.h"
#include <wtf/PassRefPtr.h>
#include <wtf/Threading.h>
#include <wtf/RefCounted.h>
namespace WebCore {
......@@ -42,7 +42,7 @@ namespace WebCore {
enum StorageType { LocalStorage, SessionStorage };
// This interface is required for Chromium since these actions need to be proxied between processes.
class StorageArea : public ThreadSafeShared<StorageArea> {
class StorageArea : public RefCounted<StorageArea> {
public:
virtual ~StorageArea() { }
......
......@@ -40,6 +40,7 @@ namespace WebCore {
StorageAreaImpl::~StorageAreaImpl()
{
ASSERT(isMainThread());
}
PassRefPtr<StorageAreaImpl> StorageAreaImpl::create(StorageType storageType, PassRefPtr<SecurityOrigin> origin, PassRefPtr<StorageSyncManager> syncManager)
......@@ -56,6 +57,7 @@ StorageAreaImpl::StorageAreaImpl(StorageType storageType, PassRefPtr<SecurityOri
, m_isShutdown(false)
#endif
{
ASSERT(isMainThread());
ASSERT(m_securityOrigin);
ASSERT(m_storageMap);
......@@ -82,6 +84,7 @@ StorageAreaImpl::StorageAreaImpl(StorageAreaImpl* area)
, m_isShutdown(area->m_isShutdown)
#endif
{
ASSERT(isMainThread());
ASSERT(m_securityOrigin);
ASSERT(m_storageMap);
ASSERT(!m_isShutdown);
......
......@@ -57,6 +57,7 @@ StorageAreaSync::StorageAreaSync(PassRefPtr<StorageSyncManager> storageSyncManag
, m_syncScheduled(false)
, m_importComplete(false)
{
ASSERT(isMainThread());
ASSERT(m_storageArea);
ASSERT(m_syncManager);
......@@ -68,6 +69,7 @@ StorageAreaSync::StorageAreaSync(PassRefPtr<StorageSyncManager> storageSyncManag
StorageAreaSync::~StorageAreaSync()
{
ASSERT(isMainThread());
ASSERT(!m_syncTimer.isActive());
}
......
......@@ -50,6 +50,7 @@ PassRefPtr<StorageSyncManager> StorageSyncManager::create(const String& path)
StorageSyncManager::StorageSyncManager(const String& path)
: m_path(path.copy())
{
ASSERT(isMainThread());
ASSERT(!m_path.isEmpty());
m_thread = LocalStorageThread::create();
m_thread->start();
......@@ -57,6 +58,7 @@ StorageSyncManager::StorageSyncManager(const String& path)
StorageSyncManager::~StorageSyncManager()
{
ASSERT(isMainThread());
}
String StorageSyncManager::fullDatabaseFilename(SecurityOrigin* origin)
......@@ -85,7 +87,7 @@ bool StorageSyncManager::scheduleImport(PassRefPtr<StorageAreaSync> area)
ASSERT(isMainThread());
if (m_thread)
m_thread->scheduleImport(area);
m_thread->scheduleImport(area.get());
return m_thread;
}
......@@ -95,7 +97,7 @@ void StorageSyncManager::scheduleSync(PassRefPtr<StorageAreaSync> area)
ASSERT(isMainThread());
if (m_thread)
m_thread->scheduleSync(area);
m_thread->scheduleSync(area.get());
}
} // namespace WebCore
......
......@@ -31,8 +31,8 @@
#include "PlatformString.h"
#include <wtf/PassRefPtr.h>
#include <wtf/RefCounted.h>
#include <wtf/RefPtr.h>
#include <wtf/Threading.h>
namespace WebCore {
......@@ -40,7 +40,7 @@ namespace WebCore {
class SecurityOrigin;
class StorageAreaSync;
class StorageSyncManager : public ThreadSafeShared<StorageSyncManager> {
class StorageSyncManager : public RefCounted<StorageSyncManager> {
public:
static PassRefPtr<StorageSyncManager> create(const String& path);
~StorageSyncManager();
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment