735778 - Call MOZ_ASSERT if RemoveObserver is called with an observer that is not present

Status: RESOLVED FIXED

Description

[Mounir Lamouri (:mounir)]

Follow-up from bug 725951. So developers will know if they are doing something wrong with {Add,Remove}Observers.

Comment 1

[Mounir Lamouri (:mounir)]

Created attachment 605969 [details] [diff] [review]
Patch v1

Comment 2

[Justin Lebar (not reading bugmail)]

Perhaps you should use MOZ_ASSERT, which is an actual fatal (debug) assert?

Comment 3

[Mounir Lamouri (:mounir)]

(In reply to Justin Lebar [:jlebar] from comment #2)
> Perhaps you should use MOZ_ASSERT, which is an actual fatal (debug) assert?

I think NS_ASSERTION or MOZ_ASSERT would be wrong. It's not fatal to happen to try to remove an observer that is not present. It's clearly an error but the program might work just well.

Comment 4

[Mounir Lamouri (:mounir)]

Comment on attachment 605969 [details] [diff] [review]
Patch v1

I realize that Justin can definitely review that and I'm pretty sure Chris has a big review queue.

Comment 5

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

I don't mind reviewing, but I'm happy for jlebar to review.

I agree with jlebar: I think this should be MOZ_ASSERT, while we still consider it a logic error.  NS_ERROR is next to worthless IME.  If we want to emasculate the check, we should do so because we no longer think it's a logic error.

Comment 6

[Justin Lebar (not reading bugmail)]

Comment on attachment 605969 [details] [diff] [review]
Patch v1

r-, since Chris and I seem to be in agreement.

Comment 7

[Mounir Lamouri (:mounir)]

I think MOZ_ASSERT is a bit extreme. Can we find a compromise with NS_ASSERTION?

Comment 8

[Mounir Lamouri (:mounir)]

Created attachment 606281 [details] [diff] [review]
Patch v2

Is NS_ASSERTION okay?

I'm holding landing a few patches to make sure this land with

Comment 9

[Mounir Lamouri (:mounir)]

... with the patch that remove MOZ_ASSERT when mObservers is null.

Comment 10

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

Comment on attachment 606281 [details] [diff] [review]
Patch v2

This isn't a compromise :).  NS_ASSERTION is the same as NS_ERROR, neutered.

Can you please explain why you "feel" that NS_ASSERTION is more appropriate than MOZ_ASSERT?

Comment 11

[Justin Lebar (not reading bugmail)]

Personally, I'd rather have no assertion at all than NS_ASSERTION or NS_ERROR.  No assertion falls under the guise of "be forgiving in what you accept, but strict in what you output," whereas an NS_ASSERTION means "eh, I don't think this invariant is actually important."

Comment 12

[Mounir Lamouri (:mounir)]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #10)
> This isn't a compromise :).  NS_ASSERTION is the same as NS_ERROR, neutered.

I didn't know, I thought NS_ERROR() was like NS_WARNING, showing "ERROR:" but it seems like not. I will now stop writing NS_ASSERTION(false, "") ;)

IMO, MOZ_ASSERT should be used when the program is in a critical state. Something that should not happen and if that happen, we can't guarantee what is going to happen next. I don't see why we should stop everything if someone tries to remove an observer that is not present in the list. It's a logical error but the program will more than likely run as expected.

Given that NS_ERROR and NS_ASSERTION are actually the same thing, I would go for NS_WARNING or NS_ERROR/NS_ASSERTION. I don't want to waste too much time on that though.

(In reply to Justin Lebar [:jlebar] from comment #11)
> Personally, I'd rather have no assertion at all than NS_ASSERTION or
> NS_ERROR.  No assertion falls under the guise of "be forgiving in what you
> accept, but strict in what you output," whereas an NS_ASSERTION means "eh, I
> don't think this invariant is actually important."

I think asserting (or warning) will help developers to fix what they are doing assuming they are doing something wrong.

Comment 13

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

Here's basically how this works, IME
 MOZ_ASSERT > devs are notified of logic errors
 [anything else] > /dev/null [*]

I think we still currently agree that these cases are logic errors.  So can we agree on MOZ_ASSERT?

[*] except for NS_ASSERTION/NS_ERROR in the reftest harness, which are counted

Comment 14

[Mounir Lamouri (:mounir)]

Created attachment 606600 [details] [diff] [review]
Patch v3.1

Using MOZ_ASSERT.

Comment 15

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

Comment on attachment 606600 [details] [diff] [review]
Patch v3.1

diff --git a/hal/Hal.cpp b/hal/Hal.cpp

>+    MOZ_ASSERT(mObservers && mObservers->RemoveObserver(aObserver));
> 

MOZ_ASSERT is a no-op in opt builds.

Comment 16

[Mounir Lamouri (:mounir)]

Created attachment 606606 [details] [diff] [review]
Patch v3.1

Oups...

Comment 17

[Justin Lebar (not reading bugmail)]

Comment on attachment 606606 [details] [diff] [review]
Patch v3.1

mounir is itching to get this in...

> +    // In addition, if RemoveObserver() returns false that means we didn't
> +    // find the observer.
> +    // In both case, that is a logical error we want to make sure the developer
> +    // notices.

Nit: s/returns false/returns false,/
Nit: s/In both case,/In both cases,/

Comment 18

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

Thanks jlebar!

Comment 19

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/0d61a0d8dba4

Comment 20

[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 21

[Justin Lebar (not reading bugmail)]

This patch should have had no effect on debug builds.  I'd be very surprised if it were the culprit.

Comment 22

[Justin Lebar (not reading bugmail)]

btw, Mounir -- and this is my mistake for missing it -- this patch will crash in release builds if !mObservers, but will work fine if mObservers is non-empty.  I'd be OK with a follow-up adding back |if (!mObservers) { return; }|.

Comment 23

[Mounir Lamouri (:mounir)]

Would this be okay:
#ifndef DEBUG
  if (!mObservers) {
    return;
  }
#endif

Comment 24

[Justin Lebar (not reading bugmail)]

I'd be happy with or without the ifndef.

Comment 25

[Marco Bonardo [::mak]]

https://hg.mozilla.org/mozilla-central/rev/436f102861fc