diff options
-rw-r--r-- | base/memory/weak_ptr.cc | 14 | ||||
-rw-r--r-- | base/memory/weak_ptr.h | 55 | ||||
-rw-r--r-- | base/memory/weak_ptr_unittest.cc | 60 | ||||
-rw-r--r-- | chrome/browser/extensions/api/declarative/rules_registry_with_cache.cc | 3 | ||||
-rw-r--r-- | chrome/browser/io_thread.cc | 5 | ||||
-rw-r--r-- | chrome/browser/plugins/plugin_info_message_filter.cc | 2 | ||||
-rw-r--r-- | chrome/browser/search_engines/search_provider_install_data.cc | 1 | ||||
-rw-r--r-- | net/proxy/dhcp_proxy_script_adapter_fetcher_win.cc | 5 | ||||
-rw-r--r-- | net/proxy/dhcp_proxy_script_fetcher_win.cc | 5 | ||||
-rw-r--r-- | remoting/host/policy_hack/policy_watcher_linux.cc | 12 | ||||
-rw-r--r-- | ui/gl/gl_surface_glx.cc | 3 | ||||
-rw-r--r-- | webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc | 11 | ||||
-rw-r--r-- | win8/test/ui_automation_client.cc | 6 |
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() { |