Fix bug in ObserverListThreadsafe::RemoveObserver

Fix bug in ObserverListThreadsafe::RemoveObserver

Like ObserverList, ObserverListThreadSafe::RemoveObserver should do
nothing if the observer isn't in the list.  Instead, it causes weird
bugs if the list has 0 or 1 existing elements.

Add comments clarifying semantics of AddObserver/RemoveObserver.

Change AddObserver to do nothing in release mode if it tries to add an existing observer.

BUG=84922
TEST=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=87944

[akalin, 2011-06-04 03:37:13]

+willchan for review

[willchan no longer on Chromium, 2011-06-04 18:01:19]

You've definitely made the implementations consistent, and this is good, but I'm
not sure that I like this semantic. I am generally biased towards APIs being
stricter and not allowing unintended use. Do you need this semantic? I think
it's probably a bug to call RemoveObserver with an observer that was never
added. I think it should crash in debug mode. It is fine to be resilient against
this in release builds, but I think I'd prefer to crash in debug builds. WDYT?

[akalin, 2011-06-04 18:56:33]

On 2011/06/04 18:01:19, willchan wrote:
> You've definitely made the implementations consistent, and this is good, but
I'm
> not sure that I like this semantic. I am generally biased towards APIs being
> stricter and not allowing unintended use. Do you need this semantic? I think
> it's probably a bug to call RemoveObserver with an observer that was never
> added. I think it should crash in debug mode. It is fine to be resilient
against
> this in release builds, but I think I'd prefer to crash in debug builds. WDYT?

The use case where I was running into this was where you have a class with a
Start() and Stop() method, and Start() adds the object as an observer to
something and Stop() removes the object as an observer (plus any cleanup).  It's
nice to be able to go:

Start() {
  Stop();  // Clear out any state.
  // Initialize new state.
}

Also, ObserverList has allowed RemoveObserver for non-existent observers since
forever.  I'm worried that changing that semantic will break lots of code.

Since this fixes an actual bug, WDYT about checking in this fix, and then
changing the semantics for both in a future CL?  I can add a TODO/bug somewhere.

[willchan no longer on Chromium, 2011-06-04 19:14:02]

On Sat, Jun 4, 2011 at 8:56 PM,  <akalin@chromium.org> wrote:
> On 2011/06/04 18:01:19, willchan wrote:
>>
>> You've definitely made the implementations consistent, and this is good,
>> but
>
> I'm
>>
>> not sure that I like this semantic. I am generally biased towards APIs
>> being
>> stricter and not allowing unintended use. Do you need this semantic? I
>> think
>> it's probably a bug to call RemoveObserver with an observer that was never
>> added. I think it should crash in debug mode. It is fine to be resilient
>
> against
>>
>> this in release builds, but I think I'd prefer to crash in debug builds.
>> WDYT?
>
> The use case where I was running into this was where you have a class with a
> Start() and Stop() method, and Start() adds the object as an observer to
> something and Stop() removes the object as an observer (plus any cleanup).
>  It's
> nice to be able to go:
>
> Start() {
>  Stop();  // Clear out any state.
>  // Initialize new state.
> }

This argument makes sense, and I suspect it's the more popular
convention in Chromium, although I'm generally pretty anal and prefer
people being exact and not lazy/sloppy.

>
> Also, ObserverList has allowed RemoveObserver for non-existent observers
> since
> forever.  I'm worried that changing that semantic will break lots of code.

That's a very fair concern. It's definitely unreasonable to ask you to
spend time making that change. I was more saying that is my ideal.

>
> Since this fixes an actual bug, WDYT about checking in this fix, and then
> changing the semantics for both in a future CL?  I can add a TODO/bug
> somewhere.

OK, that sounds good to me. Add a TODO that mentions we should
consider it, but don't feel compelled to do it.

>
> http://codereview.chromium.org/7024037/
>

LGTM

[akalin, 2011-06-05 06:17:30]

> This argument makes sense, and I suspect it's the more popular
> convention in Chromium, although I'm generally pretty anal and prefer
> people being exact and not lazy/sloppy.

I think maybe adding HasObserver() (meant to be called on the same thread as
Add/Remove) to ThreadSafeObserverList would help.  Then stop would do:

if (HasObserver()) RemoveObserver()

instead of plain RemoveObserver().

> OK, that sounds good to me. Add a TODO that mentions we should
> consider it, but don't feel compelled to do it.

Filed a bug instead: http://crbug.com/84976.

[commit-bot: I haz the power, 2011-06-05 07:07:14]

Change committed as 87944