summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
-rw-r--r--base/memory/weak_ptr.cc14
-rw-r--r--base/memory/weak_ptr.h55
-rw-r--r--base/memory/weak_ptr_unittest.cc60
-rw-r--r--chrome/browser/extensions/api/declarative/rules_registry_with_cache.cc3
-rw-r--r--chrome/browser/io_thread.cc5
-rw-r--r--chrome/browser/plugins/plugin_info_message_filter.cc2
-rw-r--r--chrome/browser/search_engines/search_provider_install_data.cc1
-rw-r--r--net/proxy/dhcp_proxy_script_adapter_fetcher_win.cc5
-rw-r--r--net/proxy/dhcp_proxy_script_fetcher_win.cc5
-rw-r--r--remoting/host/policy_hack/policy_watcher_linux.cc12
-rw-r--r--ui/gl/gl_surface_glx.cc3
-rw-r--r--webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc11
-rw-r--r--win8/test/ui_automation_client.cc6
13 files changed, 101 insertions, 81 deletions
diff --git a/base/memory/weak_ptr.cc b/base/memory/weak_ptr.cc
index 9dec8fd..321b057 100644
--- a/base/memory/weak_ptr.cc
+++ b/base/memory/weak_ptr.cc
@@ -8,17 +8,23 @@ namespace base {
namespace internal {
WeakReference::Flag::Flag() : is_valid_(true) {
+ // Flags only become bound when checked for validity, or invalidated,
+ // so that we can check that later validity/invalidation operations on
+ // the same Flag take place on the same thread.
+ thread_checker_.DetachFromThread();
}
void WeakReference::Flag::Invalidate() {
// The flag being invalidated with a single ref implies that there are no
// weak pointers in existence. Allow deletion on other thread in this case.
- DCHECK(thread_checker_.CalledOnValidThread() || HasOneRef());
+ DCHECK(thread_checker_.CalledOnValidThread() || HasOneRef())
+ << "WeakPtrs must be checked and invalidated on the same thread.";
is_valid_ = false;
}
bool WeakReference::Flag::IsValid() const {
- DCHECK(thread_checker_.CalledOnValidThread());
+ DCHECK(thread_checker_.CalledOnValidThread())
+ << "WeakPtrs must be checked and invalidated on the same thread.";
return is_valid_;
}
@@ -46,10 +52,10 @@ WeakReferenceOwner::~WeakReferenceOwner() {
}
WeakReference WeakReferenceOwner::GetRef() const {
- // We also want to reattach to the current thread if all previous references
- // have gone away.
+ // If we hold the last reference to the Flag then create a new one.
if (!HasRefs())
flag_ = new WeakReference::Flag();
+
return WeakReference(flag_);
}
diff --git a/base/memory/weak_ptr.h b/base/memory/weak_ptr.h
index 19def01..8c2220f 100644
--- a/base/memory/weak_ptr.h
+++ b/base/memory/weak_ptr.h
@@ -19,6 +19,9 @@
// void SpawnWorker() { Worker::StartNew(weak_factory_.GetWeakPtr()); }
// void WorkComplete(const Result& result) { ... }
// private:
+// // Member variables should appear before the WeakPtrFactory, to ensure
+// // that any WeakPtrs to Controller are invalidated before its members
+// // variable's destructors are executed, rendering them invalid.
// WeakPtrFactory<Controller> weak_factory_;
// };
//
@@ -44,17 +47,18 @@
// ------------------------- IMPORTANT: Thread-safety -------------------------
-// Weak pointers must always be dereferenced and invalidated on the same thread
-// otherwise checking the pointer would be racey. WeakPtrFactory enforces this
-// by binding itself to the current thread when a WeakPtr is first created
-// and un-binding only when those pointers are invalidated. WeakPtrs may still
-// be handed off to other threads, however, so long as they are only actually
-// dereferenced on the originating thread. This includes posting tasks to the
-// thread using base::Bind() to invoke a method on the object via the WeakPtr.
-
-// Calling SupportsWeakPtr::DetachFromThread() can work around the limitations
-// above and cancel the thread binding of the object and all WeakPtrs pointing
-// to it, but it's not recommended and unsafe. See crbug.com/232143.
+// Weak pointers may be passed safely between threads, but must always be
+// dereferenced and invalidated on the same thread otherwise checking the
+// pointer would be racey.
+//
+// To ensure correct use, the first time a WeakPtr issued by a WeakPtrFactory
+// is dereferenced, the factory and its WeakPtrs become bound to the calling
+// thread, and cannot be dereferenced or invalidated on any other thread. Bound
+// WeakPtrs can still be handed off to other threads, e.g. to use to post tasks
+// back to object on the bound thread.
+//
+// Invalidating the factory's WeakPtrs un-binds it from the thread, allowing it
+// to be passed for a different thread to use or delete it.
#ifndef BASE_MEMORY_WEAK_PTR_H_
#define BASE_MEMORY_WEAK_PTR_H_
@@ -77,7 +81,7 @@ namespace internal {
class BASE_EXPORT WeakReference {
public:
- // While Flag is bound to a specific thread, it may be deleted from another
+ // Although Flag is bound to a specific thread, it may be deleted from another
// via base::WeakPtr::~WeakPtr().
class Flag : public RefCountedThreadSafe<Flag> {
public:
@@ -86,7 +90,8 @@ class BASE_EXPORT WeakReference {
void Invalidate();
bool IsValid() const;
- void DetachFromThread() { thread_checker_.DetachFromThread(); }
+ // Remove this when crbug.com/234964 is addressed.
+ void DetachFromThreadHack() { thread_checker_.DetachFromThread(); }
private:
friend class base::RefCountedThreadSafe<Flag>;
@@ -120,10 +125,9 @@ class BASE_EXPORT WeakReferenceOwner {
void Invalidate();
- // Indicates that this object will be used on another thread from now on.
- // Do not use this in new code. See crbug.com/232143.
- void DetachFromThread() {
- if (flag_) flag_->DetachFromThread();
+ // Remove this when crbug.com/234964 is addressed.
+ void DetachFromThreadHack() {
+ if (flag_) flag_->DetachFromThreadHack();
}
private:
@@ -269,13 +273,6 @@ class WeakPtrFactory {
return weak_reference_owner_.HasRefs();
}
- // Indicates that this object will be used on another thread from now on.
- // Do not use this in new code. See crbug.com/232143.
- void DetachFromThread() {
- DCHECK(ptr_);
- weak_reference_owner_.DetachFromThread();
- }
-
private:
internal::WeakReferenceOwner weak_reference_owner_;
T* ptr_;
@@ -296,10 +293,12 @@ class SupportsWeakPtr : public internal::SupportsWeakPtrBase {
return WeakPtr<T>(weak_reference_owner_.GetRef(), static_cast<T*>(this));
}
- // Indicates that this object will be used on another thread from now on.
- // Do not use this in new code. See crbug.com/232143.
- void DetachFromThread() {
- weak_reference_owner_.DetachFromThread();
+ // Removes the binding, if any, from this object to a particular thread.
+ // This is used in WebGraphicsContext3DInProcessCommandBufferImpl to work-
+ // around access to cmmand buffer objects by more than one thread.
+ // Remove this when crbug.com/234964 is addressed.
+ void DetachFromThreadHack() {
+ weak_reference_owner_.DetachFromThreadHack();
}
protected:
diff --git a/base/memory/weak_ptr_unittest.cc b/base/memory/weak_ptr_unittest.cc
index dbe7960..ff1103c 100644
--- a/base/memory/weak_ptr_unittest.cc
+++ b/base/memory/weak_ptr_unittest.cc
@@ -339,7 +339,7 @@ TEST(WeakPtrTest, MoveOwnershipExplicitlyObjectNotReferenced) {
// Case 1: The target is not bound to any thread yet. So calling
// DetachFromThread() is a no-op.
Target target;
- target.DetachFromThread();
+ target.DetachFromThreadHack();
// Case 2: The target is bound to main thread but no WeakPtr is pointing to
// it. In this case, it will be re-bound to any thread trying to get a
@@ -347,7 +347,7 @@ TEST(WeakPtrTest, MoveOwnershipExplicitlyObjectNotReferenced) {
{
WeakPtr<Target> weak_ptr = target.AsWeakPtr();
}
- target.DetachFromThread();
+ target.DetachFromThreadHack();
}
TEST(WeakPtrTest, MoveOwnershipExplicitly) {
@@ -362,7 +362,7 @@ TEST(WeakPtrTest, MoveOwnershipExplicitly) {
EXPECT_EQ(&target, background.DeRef(arrow));
// Detach from background thread.
- target.DetachFromThread();
+ target.DetachFromThreadHack();
// Re-bind to main thread.
EXPECT_EQ(&target, arrow->target.get());
@@ -500,7 +500,7 @@ TEST(WeakPtrDeathTest, WeakPtrCopyDoesNotChangeThreadBinding) {
background.DeleteArrow(arrow_copy);
}
-TEST(WeakPtrDeathTest, NonOwnerThreadDereferencesWeakPtr) {
+TEST(WeakPtrDeathTest, NonOwnerThreadDereferencesWeakPtrAfterReference) {
// The default style "fast" does not support multi-threaded tests
// (introduces deadlock on Linux).
::testing::FLAGS_gtest_death_test_style = "threadsafe";
@@ -512,6 +512,7 @@ TEST(WeakPtrDeathTest, NonOwnerThreadDereferencesWeakPtr) {
// thread ownership can not be implicitly moved).
Arrow arrow;
arrow.target = target.AsWeakPtr();
+ arrow.target.get();
// Background thread tries to deref target, which violates thread ownership.
BackgroundThread background;
@@ -519,23 +520,66 @@ TEST(WeakPtrDeathTest, NonOwnerThreadDereferencesWeakPtr) {
ASSERT_DEATH(background.DeRef(&arrow), "");
}
-TEST(WeakPtrDeathTest, NonOwnerThreadDeletesObject) {
+TEST(WeakPtrDeathTest, NonOwnerThreadDeletesWeakPtrAfterReference) {
// The default style "fast" does not support multi-threaded tests
// (introduces deadlock on Linux).
::testing::FLAGS_gtest_death_test_style = "threadsafe";
scoped_ptr<Target> target(new Target());
- // Main thread creates an arrow referencing the Target (so target's thread
- // ownership can not be implicitly moved).
+
+ // Main thread creates an arrow referencing the Target.
+ Arrow arrow;
+ arrow.target = target->AsWeakPtr();
+
+ // Background thread tries to deref target, binding it to the thread.
+ BackgroundThread background;
+ background.Start();
+ background.DeRef(&arrow);
+
+ // Main thread deletes Target, violating thread binding.
+ Target* foo = target.release();
+ ASSERT_DEATH(delete foo, "");
+}
+
+TEST(WeakPtrDeathTest, NonOwnerThreadDeletesObjectAfterReference) {
+ // The default style "fast" does not support multi-threaded tests
+ // (introduces deadlock on Linux).
+ ::testing::FLAGS_gtest_death_test_style = "threadsafe";
+
+ scoped_ptr<Target> target(new Target());
+
+ // Main thread creates an arrow referencing the Target, and references it, so
+ // that it becomes bound to the thread.
Arrow arrow;
arrow.target = target->AsWeakPtr();
+ arrow.target.get();
- // Background thread tries to delete target, which violates thread ownership.
+ // Background thread tries to delete target, volating thread binding.
BackgroundThread background;
background.Start();
ASSERT_DEATH(background.DeleteTarget(target.release()), "");
}
+TEST(WeakPtrDeathTest, NonOwnerThreadReferencesObjectAfterDeletion) {
+ // The default style "fast" does not support multi-threaded tests
+ // (introduces deadlock on Linux).
+ ::testing::FLAGS_gtest_death_test_style = "threadsafe";
+
+ scoped_ptr<Target> target(new Target());
+
+ // Main thread creates an arrow referencing the Target.
+ Arrow arrow;
+ arrow.target = target->AsWeakPtr();
+
+ // Background thread tries to delete target, binding the object to the thread.
+ BackgroundThread background;
+ background.Start();
+ background.DeleteTarget(target.release());
+
+ // Main thread attempts to dereference the target, violating thread binding.
+ ASSERT_DEATH(arrow.target.get(), "");
+}
+
#endif
} // namespace base
diff --git a/chrome/browser/extensions/api/declarative/rules_registry_with_cache.cc b/chrome/browser/extensions/api/declarative/rules_registry_with_cache.cc
index 59ea150..3a22d5c 100644
--- a/chrome/browser/extensions/api/declarative/rules_registry_with_cache.cc
+++ b/chrome/browser/extensions/api/declarative/rules_registry_with_cache.cc
@@ -96,9 +96,6 @@ RulesRegistryWithCache::RulesRegistryWithCache(
ui_part->reset(storage_on_ui_.get());
- // After construction, |this| continues to live only on |owner_thread|.
- weak_ptr_factory_.DetachFromThread();
-
storage_on_ui_->Init();
}
diff --git a/chrome/browser/io_thread.cc b/chrome/browser/io_thread.cc
index 840d94b..bf9ba65 100644
--- a/chrome/browser/io_thread.cc
+++ b/chrome/browser/io_thread.cc
@@ -584,11 +584,6 @@ void IOThread::Init() {
FROM_HERE,
base::Bind(&IOThread::InitSystemRequestContext,
weak_factory_.GetWeakPtr()));
-
- // We constructed the weak pointer on the IO thread but it will be
- // used on the UI thread. Call this to avoid a thread checker
- // error.
- weak_factory_.DetachFromThread();
}
void IOThread::CleanUp() {
diff --git a/chrome/browser/plugins/plugin_info_message_filter.cc b/chrome/browser/plugins/plugin_info_message_filter.cc
index a83f203..5e635d6 100644
--- a/chrome/browser/plugins/plugin_info_message_filter.cc
+++ b/chrome/browser/plugins/plugin_info_message_filter.cc
@@ -105,8 +105,6 @@ bool PluginInfoMessageFilter::OnMessageReceived(const IPC::Message& message,
void PluginInfoMessageFilter::OnDestruct() const {
const_cast<PluginInfoMessageFilter*>(this)->
- weak_ptr_factory_.DetachFromThread();
- const_cast<PluginInfoMessageFilter*>(this)->
weak_ptr_factory_.InvalidateWeakPtrs();
// Destroy on the UI thread because we contain a |PrefMember|.
diff --git a/chrome/browser/search_engines/search_provider_install_data.cc b/chrome/browser/search_engines/search_provider_install_data.cc
index af77e8f..791a501 100644
--- a/chrome/browser/search_engines/search_provider_install_data.cc
+++ b/chrome/browser/search_engines/search_provider_install_data.cc
@@ -167,7 +167,6 @@ SearchProviderInstallData::SearchProviderInstallData(
// the given notification occurs.
new GoogleURLObserver(profile, new GoogleURLChangeNotifier(AsWeakPtr()),
ui_death_notification, ui_death_source);
- DetachFromThread();
}
SearchProviderInstallData::~SearchProviderInstallData() {
diff --git a/net/proxy/dhcp_proxy_script_adapter_fetcher_win.cc b/net/proxy/dhcp_proxy_script_adapter_fetcher_win.cc
index 8a405a7..2b49b94 100644
--- a/net/proxy/dhcp_proxy_script_adapter_fetcher_win.cc
+++ b/net/proxy/dhcp_proxy_script_adapter_fetcher_win.cc
@@ -41,11 +41,6 @@ DhcpProxyScriptAdapterFetcher::DhcpProxyScriptAdapterFetcher(
DhcpProxyScriptAdapterFetcher::~DhcpProxyScriptAdapterFetcher() {
Cancel();
-
- // The WeakPtr we passed to the worker thread may be destroyed on the
- // worker thread. This detaches any outstanding WeakPtr state from
- // the current thread.
- base::SupportsWeakPtr<DhcpProxyScriptAdapterFetcher>::DetachFromThread();
}
void DhcpProxyScriptAdapterFetcher::Fetch(
diff --git a/net/proxy/dhcp_proxy_script_fetcher_win.cc b/net/proxy/dhcp_proxy_script_fetcher_win.cc
index 64f5117..9e34f51 100644
--- a/net/proxy/dhcp_proxy_script_fetcher_win.cc
+++ b/net/proxy/dhcp_proxy_script_fetcher_win.cc
@@ -47,11 +47,6 @@ DhcpProxyScriptFetcherWin::DhcpProxyScriptFetcherWin(
DhcpProxyScriptFetcherWin::~DhcpProxyScriptFetcherWin() {
// Count as user-initiated if we are not yet in STATE_DONE.
Cancel();
-
- // The WeakPtr we passed to the worker thread may be destroyed on the
- // worker thread. This detaches any outstanding WeakPtr state from
- // the current thread.
- base::SupportsWeakPtr<DhcpProxyScriptFetcherWin>::DetachFromThread();
}
int DhcpProxyScriptFetcherWin::Fetch(base::string16* utf16_text,
diff --git a/remoting/host/policy_hack/policy_watcher_linux.cc b/remoting/host/policy_hack/policy_watcher_linux.cc
index 641af9c..f3cffea 100644
--- a/remoting/host/policy_hack/policy_watcher_linux.cc
+++ b/remoting/host/policy_hack/policy_watcher_linux.cc
@@ -51,12 +51,6 @@ class PolicyWatcherLinux : public PolicyWatcher {
: PolicyWatcher(task_runner),
config_dir_(config_dir),
weak_factory_(this) {
- // Detach the factory because we ensure that only the policy thread ever
- // calls methods on this. Also, the API contract of having to call
- // StopWatching() (which signals completion) after StartWatching()
- // before this object can be destructed ensures there are no users of
- // this object before it is destructed.
- weak_factory_.DetachFromThread();
}
virtual ~PolicyWatcherLinux() {}
@@ -84,8 +78,12 @@ class PolicyWatcherLinux : public PolicyWatcher {
virtual void StopWatchingInternal() OVERRIDE {
DCHECK(OnPolicyWatcherThread());
- // Cancel any inflight requests.
+
+ // Stop watching for changes to files in the policies directory.
watcher_.reset();
+
+ // Orphan any pending OnFilePathChanged tasks.
+ weak_factory_.InvalidateWeakPtrs();
}
private:
diff --git a/ui/gl/gl_surface_glx.cc b/ui/gl/gl_surface_glx.cc
index 87dffe9..b2f9cf9 100644
--- a/ui/gl/gl_surface_glx.cc
+++ b/ui/gl/gl_surface_glx.cc
@@ -240,9 +240,6 @@ class SGIVideoSyncVSyncProvider
shim_((new SGIVideoSyncProviderThreadShim(window))->AsWeakPtr()),
cancel_vsync_flag_(shim_->cancel_vsync_flag()),
vsync_lock_(shim_->vsync_lock()) {
- // The WeakPtr is bound to the SGIVideoSyncThread. We only use it for
- // PostTask.
- shim_->DetachFromThread();
vsync_thread_->message_loop()->PostTask(
FROM_HERE,
base::Bind(&SGIVideoSyncProviderThreadShim::Initialize, shim_));
diff --git a/webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc b/webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc
index dabf967..a50da29 100644
--- a/webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc
+++ b/webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc
@@ -268,10 +268,11 @@ static bool g_use_virtualized_gl_context = false;
namespace {
-// Also calls DetachFromThread on all GLES2Decoders before the lock is released
-// to maintain the invariant that all decoders are unbounded while the lock is
-// not held. This is to workaround DumpRenderTree uses WGC3DIPCBI with shared
-// resources on different threads.
+// Also calls DetachFromThreadHack on all GLES2Decoders before the lock is
+// released to maintain the invariant that all decoders are unbound while the
+// lock is not held. This is to workaround DumpRenderTree using WGC3DIPCBI with
+// shared resources on different threads.
+// Remove this as part of crbug.com/234964.
class AutoLockAndDecoderDetachThread {
public:
AutoLockAndDecoderDetachThread(base::Lock& lock,
@@ -292,7 +293,7 @@ AutoLockAndDecoderDetachThread::AutoLockAndDecoderDetachThread(
void DetachThread(GLInProcessContext* context) {
if (context->GetDecoder())
- context->GetDecoder()->DetachFromThread();
+ context->GetDecoder()->DetachFromThreadHack();
}
AutoLockAndDecoderDetachThread::~AutoLockAndDecoderDetachThread() {
diff --git a/win8/test/ui_automation_client.cc b/win8/test/ui_automation_client.cc
index 8c866308..ef742f2 100644
--- a/win8/test/ui_automation_client.cc
+++ b/win8/test/ui_automation_client.cc
@@ -158,11 +158,7 @@ HRESULT UIAutomationClient::Context::EventHandler::HandleAutomationEvent(
base::WeakPtr<UIAutomationClient::Context>
UIAutomationClient::Context::Create() {
Context* context = new Context();
- base::WeakPtr<Context> context_ptr(context->weak_ptr_factory_.GetWeakPtr());
- // Unbind from this thread so that the instance will bind to the automation
- // thread when Initialize is called.
- context->weak_ptr_factory_.DetachFromThread();
- return context_ptr;
+ return context->weak_ptr_factory_.GetWeakPtr();
}
void UIAutomationClient::Context::DeleteOnAutomationThread() {