Mark WeakLinkNode sequence checking expensive

This makes for-each-node sequence checking expensive (which seems
redundant). As a compromise we do non-"expensive" sequence checking in
ObserverList::begin(), which should provide the same level of protection
unless iterators are passed between sequences, which would be one heck
of a thing to try to do.

This accounts for about 60% of sequence checking in a profile I did way
back. I have not profiled to see how much sequence checking remains with
the sequence checking moved to begin() nor do I know the average
ObserverList size. Let's try it out.

In the same profile (though I don't remember what I profiled) sequence
checking accounted for 1.2% of cycles. Hopefully this explains some of
the performance gap between a DCHECK and regular Canary build.

Bug: 40241607
Change-Id: Id80d3363771e05e6f38c1432ae66b4c352acf8b8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5319909
Reviewed-by: François Degros <fdegros@chromium.org>
Commit-Queue: Peter Boström <pbos@chromium.org>
Reviewed-by: danakj <danakj@chromium.org>
Reviewed-by: Vasiliy Telezhnikov <vasilyt@chromium.org>
Reviewed-by: Colin Blundell <blundell@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1277148}

base/observer_list.h

@@ -253,6 +253,7 @@ class ObserverList {
   using value_type = ObserverType;
 
   const_iterator begin() const {
+    DCHECK_CALLED_ON_VALID_SEQUENCE(iteration_sequence_checker_);
     // An optimization: do not involve weak pointers for empty list.
     return observers_.empty() ? const_iterator() : const_iterator(this);
   }

base/observer_list_internal.h

@@ -156,8 +156,10 @@ class WeakLinkNode : public base::LinkNode<WeakLinkNode<ObserverList>> {
   }
 
   ObserverList* get() const {
+#if EXPENSIVE_DCHECKS_ARE_ON()
     if (list_)
       DCHECK_CALLED_ON_VALID_SEQUENCE(list_->iteration_sequence_checker_);
+#endif  // EXPENSIVE_DCHECKS_ARE_ON()
     return list_;
   }
   ObserverList* operator->() const { return get(); }

chrome/browser/ash/platform_keys/platform_keys_service.cc

@@ -57,7 +57,11 @@ PlatformKeysServiceImpl::PlatformKeysServiceImpl(
       &PlatformKeysServiceImpl::OnDelegateShutDown, base::Unretained(this)));
 }
 
-PlatformKeysServiceImpl::~PlatformKeysServiceImpl() = default;
+PlatformKeysServiceImpl::~PlatformKeysServiceImpl() {
+  // Destroy the delegate as it calls back into OnDelegateShutdown() which we
+  // should not call on a partially-destroyed `this`.
+  delegate_.reset();
+}
 
 void PlatformKeysServiceImpl::AddObserver(
     PlatformKeysServiceObserver* observer) {

chrome/browser/ash/platform_keys/platform_keys_service.h

@@ -464,10 +464,12 @@ class PlatformKeysServiceImpl final : public PlatformKeysService {
                          chromeos::platform_keys::OperationType operation_type);
   void OnDelegateShutDown();
 
-  std::unique_ptr<PlatformKeysServiceImplDelegate> const delegate_;
+  std::unique_ptr<PlatformKeysServiceImplDelegate> delegate_;
+
   // List of observers that will be notified when the service is shut down.
   base::ObserverList<PlatformKeysServiceObserver> observers_;
   bool map_to_softoken_attrs_for_testing_ = false;
+
   base::WeakPtrFactory<PlatformKeysServiceImpl> weak_factory_{this};
 };
 

chromeos/ash/components/drivefs/drivefs_host.cc

@@ -297,6 +297,10 @@ DriveFsHost::~DriveFsHost() {
     observer.OnHostDestroyed();
     observer.Reset();
   }
+
+  // Reset `mount_state_` manually to avoid accessing a partially-destructed
+  // `this` in ~MountState().
+  mount_state_.reset();
 }
 
 bool DriveFsHost::Mount() {

components/variations/variations_ids_provider.cc

@@ -233,11 +233,14 @@ bool VariationsIdsProvider::ForceDisableVariationIds(
 }
 
 void VariationsIdsProvider::AddObserver(Observer* observer) {
-  observer_list_.AddObserver(observer);
+  base::AutoLock scoped_lock(lock_);
+  CHECK(!base::Contains(observer_list_, observer), base::NotFatalUntil::M126);
+  observer_list_.push_back(observer);
 }
 
 void VariationsIdsProvider::RemoveObserver(Observer* observer) {
-  observer_list_.RemoveObserver(observer);
+  base::AutoLock scoped_lock(lock_);
+  std::erase(observer_list_, observer);
 }
 
 void VariationsIdsProvider::ResetForTesting() {
@@ -381,8 +384,8 @@ void VariationsIdsProvider::UpdateVariationIDsHeaderValue() {
       GenerateBase64EncodedProto(/*is_signed_in=*/true,
                                  /*is_first_party_context=*/true);
 
-  for (auto& observer : observer_list_) {
-    observer.VariationIdsHeaderUpdated();
+  for (auto* observer : observer_list_) {
+    observer->VariationIdsHeaderUpdated();
   }
 }
 

components/variations/variations_ids_provider.h

@@ -15,7 +15,6 @@
 #include "base/gtest_prod_util.h"
 #include "base/memory/raw_ptr.h"
 #include "base/metrics/field_trial.h"
-#include "base/observer_list.h"
 #include "base/synchronization/lock.h"
 #include "components/variations/proto/study.pb.h"
 #include "components/variations/synthetic_trials.h"
@@ -313,8 +312,9 @@ class COMPONENT_EXPORT(VARIATIONS) VariationsIdsProvider
 
   // List of observers to notify on variation ids header update.
   // NOTE this should really check observers are unregistered but due to
-  // https://crbug.com/1051937 this isn't currently possible.
-  base::ObserverList<Observer>::Unchecked observer_list_;
+  // https://crbug.com/1051937 this isn't currently possible. Note that
+  // ObserverList is sequence checked so we can't use that here.
+  std::vector<Observer*> observer_list_ GUARDED_BY(lock_);
 
   raw_ptr<const VariationsClient> variations_client_ = nullptr;
 };

gpu/ipc/client/gpu_channel_host.cc

@@ -8,6 +8,7 @@
 #include <utility>
 
 #include "base/atomic_sequence_num.h"
+#include "base/containers/contains.h"
 #include "base/functional/bind.h"
 #include "base/memory/ptr_util.h"
 #include "base/task/single_thread_task_runner.h"
@@ -212,7 +213,9 @@ GpuChannelHost::~GpuChannelHost() = default;
 
 GpuChannelHost::ConnectionTracker::ConnectionTracker() = default;
 
-GpuChannelHost::ConnectionTracker::~ConnectionTracker() = default;
+GpuChannelHost::ConnectionTracker::~ConnectionTracker() {
+  CHECK(observer_list_.empty(), base::NotFatalUntil::M126);
+}
 
 void GpuChannelHost::ConnectionTracker::OnDisconnectedFromGpuProcess() {
   is_connected_.store(false);
@@ -222,23 +225,22 @@ void GpuChannelHost::ConnectionTracker::OnDisconnectedFromGpuProcess() {
 void GpuChannelHost::ConnectionTracker::AddObserver(
     GpuChannelLostObserver* obs) {
   AutoLock lock(channel_obs_lock_);
-  observer_list_.AddObserver(obs);
-  DCHECK(!observer_list_.empty());
+  CHECK(!base::Contains(observer_list_, obs), base::NotFatalUntil::M126);
+  observer_list_.push_back(obs);
 }
 
 void GpuChannelHost::ConnectionTracker::RemoveObserver(
     GpuChannelLostObserver* obs) {
   AutoLock lock(channel_obs_lock_);
-  observer_list_.RemoveObserver(obs);
+  std::erase(observer_list_, obs);
 }
 
 void GpuChannelHost::ConnectionTracker::NotifyGpuChannelLost() {
   AutoLock lock(channel_obs_lock_);
-  for (auto& observer : observer_list_) {
-    observer.OnGpuChannelLost();
+  for (GpuChannelLostObserver* observer : observer_list_) {
+    observer->OnGpuChannelLost();
   }
-  observer_list_.Clear();
-  DCHECK(observer_list_.empty());
+  observer_list_.clear();
 }
 
 GpuChannelHost::OrderingBarrierInfo::OrderingBarrierInfo() = default;

gpu/ipc/client/gpu_channel_host.h

@@ -191,8 +191,9 @@ class GPU_EXPORT GpuChannelHost
 
     // The GpuChannelLost Monitor for LayerTreeFrameSink.
     base::Lock channel_obs_lock_;
-    base::ObserverList<GpuChannelLostObserver>::Unchecked GUARDED_BY(
-        channel_obs_lock_) observer_list_;
+    // Note that ObserverList is sequence checked so we can't use that here.
+    std::vector<GpuChannelLostObserver*> GUARDED_BY(channel_obs_lock_)
+        observer_list_;
   };
 
   // A filter used internally to route incoming messages from the IO thread