New issue
Advanced search Search tips

Issue 656465 link

Starred by 1 user

Issue metadata

Status: Archived
Owner: ----
Closed: Jan 10
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Remove ObserverList::might_have_observers()?

Project Member Reported by dcheng@chromium.org, Oct 17 2016

Issue description

There's a lot of subtle stuff that uses this, and it feels rather fragile:

Example #1:

  // If we are removing the final observer for the source, then stop
  // observing routes for it.
  // might_have_observers() is reliable here on the assumption that this call
  // is not inside the ObserverList iteration.
  routes_query->observers.RemoveObserver(observer);
  if (!routes_query->observers.might_have_observers()) 

which assumes that this method is never called while the ObserverList is being iterated.

Example #2:

  for (auto& observer : animation_observer_list_)
    observer.OnAnimationStep(args.frame_time);
  if (animation_observer_list_.might_have_observers())
    host_->SetNeedsAnimate();

which might be able to just use a local bool to track this.

Example #3:

  ListenerKey key(listener->GetName(), listener->GetType());
  ListenerMap::iterator observer_list_iterator = listeners_.find(key);

  DCHECK(observer_list_iterator != listeners_.end());
  DCHECK(observer_list_iterator->second->HasObserver(listener));

  observer_list_iterator->second->RemoveObserver(listener);

  // Remove the observer list from the map if it is empty
  if (!observer_list_iterator->second->might_have_observers()) {
    // Schedule the actual removal for later in case the listener removal
    // happens while iterating over the observer list.
    base::ThreadTaskRunnerHandle::Get()->PostTask(
        FROM_HERE, base::Bind(&MDnsClientImpl::Core::CleanupObserverList,
                              AsWeakPtr(), key));
  }

which has two conflicting aspects:
- might_have_observers() will never return false in the middle of observer iteration
- yet it uses a PostTask() to try to handle this case safely?

Example #4:

void NetLog::UpdateIsCapturing() {
  lock_.AssertAcquired();
  base::subtle::NoBarrier_Store(&is_capturing_,
                                observers_.might_have_observers() ? 1 : 0);
}

Which combines far too many fun things into one: locks, atomics, and a potentially inaccurate answer from might_have_observers().

Strawman #1
Just replace all these with observers.begin() != observers.end(). Disadvantage is that this is a bit more expensive, but it's also guaranteed to be accurate. I think the fact that it's slightly more expensive is unlikely to matter in practice?

Strawman #2
Add a callback to ObserverList that is invoked when the ObserverList transitions to/from 0 observers.

Personally, I prefer option #1 here but I'm curious if other people have thoughts.
 

Comment 1 by loyso@chromium.org, Oct 17 2016

I vote for #1.
bool ObserverList::empty() const { return begin() == end(); }

Comment 2 by danakj@chromium.org, Oct 20 2016

begin == end sgtm
Status: Archived (was: Untriaged)
Archiving P3s older than 1 year with no owner or component.

Sign in to add a comment