[SCM] WebKit Debian packaging branch, webkit-1.1, updated. upstream/1.1.15.1-1414-gc69ee75
jorlow at chromium.org
jorlow at chromium.org
Thu Oct 29 20:36:57 UTC 2009
The following commit has been merged in the webkit-1.1 branch:
commit 914f2dd10199489bafaceafadde60c6287ab91a0
Author: jorlow at chromium.org <jorlow at chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Date: Wed Sep 30 18:52:39 2009 +0000
2009-09-21 Jeremy Orlow <jorlow at 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
diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
index 3ef7402..f5c809a 100644
--- a/WebCore/ChangeLog
+++ b/WebCore/ChangeLog
@@ -1,3 +1,67 @@
+2009-09-21 Jeremy Orlow <jorlow at 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 at chromium.org>
Reviewed by Darin Fisher.
diff --git a/WebCore/storage/LocalStorageTask.cpp b/WebCore/storage/LocalStorageTask.cpp
index 4cea845..12cc083 100644
--- a/WebCore/storage/LocalStorageTask.cpp
+++ b/WebCore/storage/LocalStorageTask.cpp
@@ -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:
diff --git a/WebCore/storage/LocalStorageTask.h b/WebCore/storage/LocalStorageTask.h
index 726b41f..f03d851 100644
--- a/WebCore/storage/LocalStorageTask.h
+++ b/WebCore/storage/LocalStorageTask.h
@@ -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
diff --git a/WebCore/storage/LocalStorageThread.cpp b/WebCore/storage/LocalStorageThread.cpp
index c85d5e6..78640a9 100644
--- a/WebCore/storage/LocalStorageThread.cpp
+++ b/WebCore/storage/LocalStorageThread.cpp
@@ -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));
diff --git a/WebCore/storage/LocalStorageThread.h b/WebCore/storage/LocalStorageThread.h
index b903fb9..e9e2b58 100644
--- a/WebCore/storage/LocalStorageThread.h
+++ b/WebCore/storage/LocalStorageThread.h
@@ -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();
diff --git a/WebCore/storage/StorageArea.h b/WebCore/storage/StorageArea.h
index e0f7f06..a64d44a 100644
--- a/WebCore/storage/StorageArea.h
+++ b/WebCore/storage/StorageArea.h
@@ -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() { }
diff --git a/WebCore/storage/StorageAreaImpl.cpp b/WebCore/storage/StorageAreaImpl.cpp
index bd0cea9..020e2b8 100644
--- a/WebCore/storage/StorageAreaImpl.cpp
+++ b/WebCore/storage/StorageAreaImpl.cpp
@@ -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);
diff --git a/WebCore/storage/StorageAreaSync.cpp b/WebCore/storage/StorageAreaSync.cpp
index 2a642b4..bec45aa 100644
--- a/WebCore/storage/StorageAreaSync.cpp
+++ b/WebCore/storage/StorageAreaSync.cpp
@@ -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());
}
diff --git a/WebCore/storage/StorageSyncManager.cpp b/WebCore/storage/StorageSyncManager.cpp
index 961b9b2..416b81c 100644
--- a/WebCore/storage/StorageSyncManager.cpp
+++ b/WebCore/storage/StorageSyncManager.cpp
@@ -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
diff --git a/WebCore/storage/StorageSyncManager.h b/WebCore/storage/StorageSyncManager.h
index b8c817f..fe35e3d 100644
--- a/WebCore/storage/StorageSyncManager.h
+++ b/WebCore/storage/StorageSyncManager.h
@@ -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();
--
WebKit Debian packaging
More information about the Pkg-webkit-commits
mailing list