Reland "Mark WeakLinkNode sequence checking expensive"

This reverts commit fe5ffc2. Reason for revert: Reland with CursorController changed to use std::vector too. Original change's description: > Revert "Mark WeakLinkNode sequence checking expensive" > > This reverts commit 6f447d5. > > Reason for revert: Crashes on ChromeOS as soon as I touch the > device's touchpad > > 2024-03-26T00:58:16.588106Z FATAL chrome[5433:5496]: [sequence_checker.cc(21)] Check failed: checker.CalledOnValidSequence(&bound_at). > #0 0x56f06aa34da2 base::debug::CollectStackTrace() > #1 0x56f06aa19ba2 base::debug::StackTrace::StackTrace() > #2 0x56f06a934073 logging::LogMessage::Flush() > #3 0x56f063de6fcd logging::LogMessage::~LogMessage() > #4 0x56f06a92053c logging::(anonymous namespace)::DCheckLogMessage::~DCheckLogMessage() > #5 0x56f06a9205ae logging::(anonymous namespace)::DCheckLogMessage::~DCheckLogMessage() > #6 0x56f063dc2ba6 logging::CheckError::~CheckError() > #7 0x56f063de9d22 base::ScopedValidateSequenceChecker::ScopedValidateSequenceChecker() > #8 0x56f063de4706 base::ObserverList<>::begin() > #9 0x56f06bfa75eb ui::CursorController::SetCursorLocation() > #10 0x56f0646d9be5 ui::DrmCursor::SetCursorLocationLocked() > #11 0x56f0646da6bf ui::DrmCursor::MoveCursor() > #12 0x56f06bfd76e6 ui::GestureInterpreterLibevdevCros::OnGestureMove() > > > Original change's description: > > 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 <[email protected]> > > Commit-Queue: Peter Boström <[email protected]> > > Reviewed-by: danakj <[email protected]> > > Reviewed-by: Vasiliy Telezhnikov <[email protected]> > > Reviewed-by: Colin Blundell <[email protected]> > > Cr-Commit-Position: refs/heads/main@{#1277148} > > Bug: 40241607 > Change-Id: I8e11a7d796ba10a38453dff336f46e3aba04ab97 > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5394849 > Bot-Commit: Rubber Stamper <[email protected]> > Owners-Override: François Degros <[email protected]> > Commit-Queue: François Degros <[email protected]> > Cr-Commit-Position: refs/heads/main@{#1278133} Bug: 40241607 Change-Id: I7aedf2194f8cc17a56ffa79c1824e33a7072c1f1 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5406313 Reviewed-by: danakj <[email protected]> Reviewed-by: Colin Blundell <[email protected]> Reviewed-by: Michael Spang <[email protected]> Reviewed-by: Vasiliy Telezhnikov <[email protected]> Reviewed-by: François Degros <[email protected]> Commit-Queue: Peter Boström <[email protected]> Cr-Commit-Position: refs/heads/main@{#1281466}
jamlee-t · Apr 2, 2024 · 7fbc138 · 7fbc138
1 parent e690bcb
commit 7fbc138
Show file tree

Hide file tree

Showing 11 changed files with 54 additions and 26 deletions.
diff --git a/base/observer_list.h b/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);
   }

diff --git a/base/observer_list_internal.h b/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(); }

diff --git a/chrome/browser/ash/platform_keys/platform_keys_service.cc b/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) {

diff --git a/chrome/browser/ash/platform_keys/platform_keys_service.h b/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};
 };
 

diff --git a/chromeos/ash/components/drivefs/drivefs_host.cc b/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() {

diff --git a/components/variations/variations_ids_provider.cc b/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();
   }
 }
 

diff --git a/components/variations/variations_ids_provider.h b/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;
 };

diff --git a/gpu/ipc/client/gpu_channel_host.cc b/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;

diff --git a/gpu/ipc/client/gpu_channel_host.h b/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

diff --git a/ui/events/ozone/chromeos/cursor_controller.cc b/ui/events/ozone/chromeos/cursor_controller.cc
@@ -4,6 +4,9 @@
 
 #include "ui/events/ozone/chromeos/cursor_controller.h"
 
+#include "base/check.h"
+#include "base/containers/contains.h"
+
 namespace ui {
 
 namespace {
@@ -46,12 +49,14 @@ CursorController* CursorController::GetInstance() {
 
 void CursorController::AddCursorObserver(CursorObserver* observer) {
   base::AutoLock lock(cursor_observers_lock_);
-  cursor_observers_.AddObserver(observer);
+  CHECK(!base::Contains(cursor_observers_, observer),
+        base::NotFatalUntil::M126);
+  cursor_observers_.push_back(observer);
 }
 
 void CursorController::RemoveCursorObserver(CursorObserver* observer) {
   base::AutoLock lock(cursor_observers_lock_);
-  cursor_observers_.RemoveObserver(observer);
+  std::erase(cursor_observers_, observer);
 }
 
 void CursorController::SetCursorConfigForWindow(
@@ -79,8 +84,9 @@ void CursorController::ApplyCursorConfigForWindow(gfx::AcceleratedWidget widget,
 
 void CursorController::SetCursorLocation(const gfx::PointF& location) {
   base::AutoLock lock(cursor_observers_lock_);
-  for (auto& observer : cursor_observers_)
-    observer.OnCursorLocationChanged(location);
+  for (auto* observer : cursor_observers_) {
+    observer->OnCursorLocationChanged(location);
+  }
 }
 
 CursorController::CursorController() {

diff --git a/ui/events/ozone/chromeos/cursor_controller.h b/ui/events/ozone/chromeos/cursor_controller.h
@@ -6,11 +6,12 @@
 #define UI_EVENTS_OZONE_CHROMEOS_CURSOR_CONTROLLER_H_
 
 #include <map>
+#include <vector>
 
 #include "base/component_export.h"
 #include "base/memory/singleton.h"
-#include "base/observer_list.h"
 #include "base/synchronization/lock.h"
+#include "base/thread_annotations.h"
 #include "ui/display/display.h"
 #include "ui/events/platform_event.h"
 #include "ui/gfx/geometry/point_f.h"
@@ -91,11 +92,13 @@ class COMPONENT_EXPORT(EVENTS_OZONE) CursorController {
   typedef std::map<gfx::AcceleratedWidget, PerWindowCursorConfiguration>
       WindowToCursorConfigurationMap;
 
-  WindowToCursorConfigurationMap window_to_cursor_configuration_map_;
   mutable base::Lock window_to_cursor_configuration_map_lock_;
+  WindowToCursorConfigurationMap window_to_cursor_configuration_map_
+      GUARDED_BY(window_to_cursor_configuration_map_lock_);
 
-  base::ObserverList<CursorObserver>::Unchecked cursor_observers_;
   mutable base::Lock cursor_observers_lock_;
+  std::vector<CursorObserver*> cursor_observers_
+      GUARDED_BY(cursor_observers_lock_);
 };
 
 }  // namespace ui