Remove all but one use of WeakPtrFactory::DetachFromThread.

This CL changes WeakPtr in the following ways:
* Changes thread-bindings semantics so that WeakPtrs only become bound when the first one is dereferenced, or the owning factory invalidates them.
* Removes WeakPtrFactory::DetachFromThread.
* Renames SupportsWeakPtr::DetachFromThread  to DetachFromThreadHack.

Calling code changes to allow this:
* Unnecessary DetachFromThread() calls removed from PluginInfoMessageFilter, DhcpProxyScript[Adapter]FetcherWin and (Chromoting's) PolicyWatcherLinux.
* DetachFromThread() calls rendered unnecessary by change in binding semantics removed from IOThread, SearchProviderInstallData, RuleRegistryWithCache and GLSurfaceGlx.

WebGraphicsContext3DInProcessCommandBufferImpl uses the re-named DetachFromThreadHack() - bug 234964 tracks work to remove that use.

BUG=232143, 234964

Review URL: https://chromiumcodereview.appspot.com/14299011

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@202038 0039d316-1c4b-4281-b951-d872f2087c98

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_);
 }
 

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:

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

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();
 }
 

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() {

chrome/browser/plugins/plugin_info_message_filter.cc

@@ -104,8 +104,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();
 

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() {

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(

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,

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:

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_));

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() {

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() {