725951 - Don't assert if mObservers is null when removing an observer from ObserversManager

Status: RESOLVED FIXED

(Core :: Hardware Abstraction Layer (HAL), defect)

Description

[Mounir Lamouri (:mounir)]

Attachment: Patch

This check was quite too cautious/extreme/pedantic: if you have 1+ observers and try to remove one that is not part of the list, nothing will happen but if you have 0 observers and try to do the same thing, it will crash. We should try to not remove observers that do not exist (to prevent looking inside the array for nothing) but we should definitely not crash in that case.

It happens that nsScreen.cpp modifications for screen orientation was triggering that (fatal) assertion.

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Why was nsScreen trying to remove an observer that didn't exist?

Comment 2

[Mounir Lamouri (:mounir)]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #1)
> Why was nsScreen trying to remove an observer that didn't exist?

Because nsScreen::Invalidate() removes |this| from observing screen orientation changes and the dtor do the same thing. The dtor does that mostly to be safe and having a boolean to keep track of the removal state seems a bit too much given that removing something that has been already removed could be considered as a no-op.

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

What's nsScreen::Invalidate()?  Why does it need to remove itself from the observer list?

Comment 4

[Mounir Lamouri (:mounir)]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #3)
> What's nsScreen::Invalidate()?  Why does it need to remove itself from the
> observer list?

nsScreen::Invalidate() does some cleanup in the method. It is removing itself from the observer list to make sure we don't get notifications between that moment and the dtor being called. Maybe it wouldn't hurt if notifications were sent. We could try.

Anyhow, I don't think that bug is related to nsScreen::Invalidate. Calling RemoveObserver when there are no observers is something that could happen and shouldn't be a fatal assert. Maybe we could warn if we try to remove an observer that is not present in general. I doubt we should do something specific for the case of there are no listed observers compared to the one where there is at least on observer but not the one we try to remove.

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I'm curious what Invalidate() is supposed to do.  It has a traditional meaning in graphical systems, which is "make the pixels all dirty so that they need to be repainted".

Why is your Destroy() method sometimes not called?

As you can tell, I'm loath to work around what seems to me to be logic errors in other parts of the code in ObserverList.  Convince me this isn't a logic error in your nsScreen patches.

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 596003 [details] [diff] [review]
Patch

Getting this out of my review queue until we finish the background discussion.

Comment 7

[Mounir Lamouri (:mounir)]

Comment on attachment 596003 [details] [diff] [review]
Patch

The fix isn't really related to nsScreen even if nsScreen::Invalidate might be broken. The issue is that we currently do a runtime abort if we call RemoveObserver(foo) when there are no observers but we don't if we call RemoveObserver(foo) and foo isn't a currently registered observer. We shouldn't have such a difference in behavior IMO. In addition, we shouldn't do a runtime abort for that. That seems pretty extreme. I would be okay to put a warning.

Now, nsScreen logic might be broken indeed. I could open a follow-up to fix it but I think it's not really the problem here.

Comment 8

[Justin Lebar (not reading bugmail)]

Comment on attachment 596003 [details] [diff] [review]
Patch

Mounir asked me to review this for him.

Comment 9

[Justin Lebar (not reading bugmail)]

I agree with comment 0 that this is problematic:

> if you have 1+ observers and try to remove one that is not part of the list, nothing will happen 
> but if you have 0 observers and try to do the same thing, it will crash.

Chris, are you OK making the behavior when we have 0 observers match the behavior when we have one or more observer?  That's what I see this patch as doing, separate from bugs on nsScreen.

r=me on this bugfix, provided Chris is happy with the state of followups that we're not ignoring other bugs.

Comment 10

[Justin Lebar (not reading bugmail)]

Comment on attachment 596003 [details] [diff] [review]
Patch

> +    // If mObservers is null, that means there are no observers so removing one
> +    // must be a no-op.

Nit: s/ so/, so/

("removing one must be a no-op" is an independent clause -- it could stand as a complete sentence.  So is "If mObservers is null, that means there are no observers."  When we connect two independent clauses using a conjunction ("so"), we use a comma.  Although I'm sure there are exceptions.)

Comment 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I'm not so happy with quietly ignoring what I would usually think are logic errors but I agree the behavior should be consistent.  Unlike nsTArray, which in general is a collection of *values*, observer arrays are a collection of unique *things*.  Things only enter the list be registering themselves.

If someone could demonstrate to me a case in which teardown logic was tremendously complicated by knowing whether an addobserver failed (which hasn't been done here), that would be a strong argument for no-op remove on empty list.

Either, fine with me for now, ++ for consistent behavior.

Comment 12

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/7e28d1a2c648

Comment 13

[Mark Finkle (:mfinkle) (use needinfo?)]

I backed this out along with all the other patches in the initial push. Something in the push regressed Ts on Android by 20% and I don't know which patch is to blame:

https://hg.mozilla.org/mozilla-central/rev/e94edfdb1f5b

Regression Ts increase 20.6% on Android 2.2 Mozilla-Inbound
--------------------------------------------------------------
    Previous: avg 2653.656 stddev 87.856 of 30 runs up to revision 1239bd0779a6
    New     : avg 3201.178 stddev 110.202 of 5 runs since revision 0d61a0d8dba4
    Change  : +547.522 (20.6% / z=6.232)
    Graph   : http://mzl.la/zD3EWy

Comment 14

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/60b8a96614c8