Closed Bug 1091174 Opened 7 years ago Closed 7 years ago

Automounter destructor call UnregisterObserver too many times

Categories

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

x86_64
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
2.1 S8 (7Nov)

People

(Reporter: dhylands, Assigned: dhylands)

Details

Attachments

(1 file)

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.
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.
Target Milestone: --- → 2.1 S8 (7Nov)
Comment on attachment 8515399 [details] [diff] [review]
Remove extra calls to Register/UnregisterObserver

A fairly small review.
Attachment #8515399 - Flags: review?(mhabicher)
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.
Attachment #8515399 - Flags: review?(mhabicher) → review+
(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.
You need to log in before you can comment on or make changes to this bug.