base::CheckedObserver: Follow-up review comments

Follow-ups to review comments in https://crrev.com/c/1053338

Bug: 842987
Change-Id: I1cf78667887bb86555d3f9043e2afe5d8eacf2bb
Reviewed-on: https://chromium-review.googlesource.com/1184590Reviewed-by: Wez <wez@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Commit-Queue: Trent Apted <tapted@chromium.org>
Cr-Commit-Position: refs/heads/master@{#585327}

base/memory/weak_ptr.h

@@ -270,7 +270,7 @@ class WeakPtr : public internal::WeakPtrBase {
 
   // Returns whether the object |this| points to has been invalidated. This can
   // be used to distinguish a WeakPtr to a destroyed object from one that has
-  // been explicitly set to null. A null WeakPtr is always valid.
+  // been explicitly set to null.
   bool WasInvalidated() const { return ptr_ && !ref_.IsValid(); }
 
  private:

base/memory/weak_ptr_unittest.cc

@@ -393,27 +393,31 @@ TEST(WeakPtrTest, InvalidateWeakPtrs) {
 }
 
 // Tests that WasInvalidated() is true only for invalidated WeakPtrs (not
-// nullptr) and doesn't DCHECK.
+// nullptr) and doesn't DCHECK (e.g. because of a dereference attempt).
 TEST(WeakPtrTest, WasInvalidatedByFactoryDestruction) {
   WeakPtr<int> ptr;
   EXPECT_FALSE(ptr.WasInvalidated());
 
-  // Test |data| destroyed.
+  // Test |data| destroyed. That is, the typical pattern when |data| (and its
+  // associated factory) go out of scope.
   {
-    int data;
+    int data = 0;
     WeakPtrFactory<int> factory(&data);
     ptr = factory.GetWeakPtr();
 
+    // Verify that a live WeakPtr is not reported as Invalidated.
     EXPECT_FALSE(ptr.WasInvalidated());
   }
-  EXPECT_TRUE(ptr.WasInvalidated());  // Shouldn't tickle asan.
+
+  // Checking validity shouldn't read beyond the stack frame.
+  EXPECT_TRUE(ptr.WasInvalidated());
   ptr = nullptr;
   EXPECT_FALSE(ptr.WasInvalidated());
 }
 
 // As above, but testing InvalidateWeakPtrs().
 TEST(WeakPtrTest, WasInvalidatedByInvalidateWeakPtrs) {
-  int data;
+  int data = 0;
   WeakPtrFactory<int> factory(&data);
   WeakPtr<int> ptr = factory.GetWeakPtr();
   EXPECT_FALSE(ptr.WasInvalidated());
@@ -423,9 +427,10 @@ TEST(WeakPtrTest, WasInvalidatedByInvalidateWeakPtrs) {
   EXPECT_FALSE(ptr.WasInvalidated());
 }
 
-// Test WasInvalidated() when assigning null before invalidating.
+// A WeakPtr should not be reported as 'invalidated' if nullptr was assigned to
+// it.
 TEST(WeakPtrTest, WasInvalidatedWhilstNull) {
-  int data;
+  int data = 0;
   WeakPtrFactory<int> factory(&data);
   WeakPtr<int> ptr = factory.GetWeakPtr();
   EXPECT_FALSE(ptr.WasInvalidated());

base/observer_list.h

@@ -104,8 +104,8 @@ class ObserverList : public SupportsWeakPtr<ObserverList<ObserverType,
  public:
   // Allow declaring an ObserverList<...>::Unchecked that replaces the default
   // ObserverStorageType to use raw pointers. This is required to support legacy
-  // observers that do not inherit from CheckedObserver. The majority of NEW
-  // CODE SHOULD NOT USE THIS, but it may be suited for performance-critical
+  // observers that do not inherit from CheckedObserver. The majority of new
+  // code should not use this, but it may be suited for performance-critical
   // situations to avoid overheads of a CHECK(). Note the type can't be chosen
   // based on ObserverType's definition because ObserverLists are often declared
   // in headers using a forward-declare of ObserverType.
@@ -285,8 +285,8 @@ class ObserverList : public SupportsWeakPtr<ObserverList<ObserverType,
   // Determine whether a particular observer is in the list.
   bool HasObserver(const ObserverType* obs) const {
     // Client code passing null could be confused by the treatment of observers
-    // removed mid-iteration. TODO(tapted): This should probably DCHECK, but
-    // some client code currently does pass null to HasObserver().
+    // removed mid-iteration. TODO(https://crbug.com/876588): This should
+    // probably DCHECK, but some client code currently does pass null.
     if (obs == nullptr)
       return false;
     return std::find_if(observers_.begin(), observers_.end(),

base/observer_list_internal.h

@@ -39,8 +39,6 @@ class BASE_EXPORT UncheckedObserverAdapter {
  private:
   void* ptr_;
 
-  // Although copying works, disallow it to be consistent with
-  // CheckedObserverAdapter.
   DISALLOW_COPY_AND_ASSIGN(UncheckedObserverAdapter);
 };
 
@@ -61,7 +59,7 @@ class BASE_EXPORT CheckedObserverAdapter {
 
   void MarkForRemoval() {
     DCHECK(weak_ptr_);
-    weak_ptr_.reset();
+    weak_ptr_ = nullptr;
   }
 
   bool IsMarkedForRemoval() const {