1091174 - Automounter destructor call UnregisterObserver too many times

Status: RESOLVED FIXED

(Firefox OS Graveyard :: MTP/UMS, defect)

Description

[Dave Hylands [:dhylands]]

I discovered this while investigating bug 1089706.

I don't think that it's a serious issue, since nsTArray::RemoveElement just returns false if the element doesn't exist.

Comment 1

[Dave Hylands [:dhylands]]

Attachment: Remove extra calls to Register/UnregisterObserver

I also did some cleanup:
- renamed RegisterObserver to RegisterVolumeObserver
- Added some debug printf for Register/Unregister/Broadcast
- renamed mEventObserverList to sEventObserverList since it's actually a static member variable.

Comment 2

[Dave Hylands [:dhylands]]

Comment on attachment 8515399 [details] [diff] [review]
Remove extra calls to Register/UnregisterObserver

A fairly small review.

Comment 3

[Mike Habicher [:mikeh] (high bugzilla latency)]

Comment on attachment 8515399 [details] [diff] [review]
Remove extra calls to Register/UnregisterObserver

Review of attachment 8515399 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM.

::: dom/system/gonk/Volume.cpp
@@ +18,5 @@
> +void
> +VolumeObserverList::Broadcast(Volume* const& aVolume)
> +{
> +  uint32_t size = mObservers.Length();
> +  for (uint32_t i = 0; i < size; ++i) {

I've been trying to use nsTArray<...>::size_type in these cases, but it's kind of unwieldy. I'll leave it up to you to decide if the abstraction is worth it.

::: dom/system/gonk/Volume.h
@@ +28,5 @@
> +
> +#define DEBUG_VOLUME_OBSERVER 0
> +
> +#if DEBUG_VOLUME_OBSERVER
> +class VolumeObserverList : public mozilla::ObserverList<Volume *>

nit: 'Volume*'

@@ +34,5 @@
> +public:
> +  void Broadcast(Volume* const& aVolume);
> +};
> +#else
> +typedef mozilla::ObserverList<Volume *> VolumeObserverList;

nit: ditto.

Comment 4

[Dave Hylands [:dhylands]]

(In reply to Mike Habicher [:mikeh] from comment #3)
> ::: dom/system/gonk/Volume.cpp
> @@ +18,5 @@
> > +void
> > +VolumeObserverList::Broadcast(Volume* const& aVolume)
> > +{
> > +  uint32_t size = mObservers.Length();
> > +  for (uint32_t i = 0; i < size; ++i) {
> 
> I've been trying to use nsTArray<...>::size_type in these cases, but it's
> kind of unwieldy. I'll leave it up to you to decide if the abstraction is
> worth it.

In this particular case I wanted to copy/paste the code from Observer.h and make minimal changes to it.

Comment 5

[Dave Hylands [:dhylands]]

 https://hg.mozilla.org/integration/b2g-inbound/rev/1a06383d7e30

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1a06383d7e30