Rename/re-comment tests and added new death tests for WeakPtr

base/memory/weak_ptr.h

Index: base/memory/weak_ptr.h
===================================================================
--- base/memory/weak_ptr.h	(revision 145398)
+++ base/memory/weak_ptr.h	(working copy)
@@ -7,15 +7,6 @@
 // bound to the lifetime of the referrers.  In other words, this is useful when
 // reference counting is not a good fit.
 //
-// Thread-safety notes:
-// When you get a WeakPtr (from a WeakPtrFactory or SupportsWeakPtr),
-// the WeakPtr becomes bound to the current thread. You may only
-// dereference the WeakPtr on that thread. However, it is safe to
-// destroy the WeakPtr object on another thread.
-// Since a WeakPtr object may be destroyed on a background thread,
-// querying WeakPtrFactory's HasWeakPtrs() method can be racy.
-//
-//
 // A common alternative to weak pointers is to have the shared object hold a
 // list of all referrers, and then when the shared object is destroyed, it
 // calls a method on the referrers to tell them to drop their references.  This
@@ -54,6 +45,22 @@
 // dereferencing the Controller back pointer after the Controller has been
 // destroyed.
 //
+// ------------------------ Thread-safety notes ------------------------
+// When you get a WeakPtr (from a WeakPtrFactory or SupportsWeakPtr), the
+// WeakPtr becomes bound to the current thread. You may only dereference the
+// WeakPtr on that thread. However, it is safe to destroy the WeakPtr object
+// on another thread. Since a WeakPtr object may be destroyed on a background
+// thread, querying WeakPtrFactory's HasWeakPtrs() method can be racy.
+//
+// On the other hand, the object that supports WeakPtr (extends SupportsWeakPtr)
+// will be bound to the first thread that creates a WeakPtr pointing to it,
+// and can not be deleted from other threads unless all WeakPtrs are deleted.

[Ryan Sleevi, 2012/07/10 22:41:54]

nit: s/unless/until/

[kaiwang, 2012/07/11 01:17:05]

Done.

+// Calling SupportsWeakPtr::DetachFromThread() can work around this and cancel
+// the thread binding, but it's not recommended and unsafe.

[Ryan Sleevi, 2012/07/10 22:41:54]

I almost wonder if we shouldn't remove DetachFromThread. I'm curious if it's
being used, and if the consumers are correct.

However, for sake of simplicitly, I think providing examples of "how to use it
correctly" may be... well.. too complex.

[kaiwang, 2012/07/11 01:17:05]

Searched the code and seems this function is used but not in many places.

+//
+// WeakPtrs are usually copied by other threads. Note the copied WeakPtr is

[Ryan Sleevi, 2012/07/10 22:41:54]

nit: s/are usually/may be/.
nit: drop the "Note"

Write the comments assuming no other code exists - talking about how it
should/can be used, not how it commonly is used.


The following may be better
"WeakPtrs may be copy-constructed, assigned, or destructed on threads other than
the thread they are bound to. However, any copies will remain bound to the
thread that the WeakPtr was originally created on, and thus may only be
dereferenced on that thread."

[kaiwang, 2012/07/11 01:17:05]

Done. Good suggestion :)

+// still bound to original thread, not the thread doing the copy. So the second
+// thread can delete the copied WeakPtr but can not dereference it.
 
 #ifndef BASE_MEMORY_WEAK_PTR_H_
 #define BASE_MEMORY_WEAK_PTR_H_
@@ -225,7 +232,8 @@
   friend class WeakPtrFactory<T>;
 
   WeakPtr(const internal::WeakReference& ref, T* ptr)
-      : WeakPtrBase(ref), ptr_(ptr) {
+      : WeakPtrBase(ref),
+        ptr_(ptr) {
   }
 
   // This pointer is only valid when ref_.is_valid() is true.  Otherwise, its