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

RESOLVED FIXED in mozilla14

Status

()

Core
Hardware Abstraction Layer (HAL)
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: mounir, Assigned: mounir)

Tracking

Trunk
mozilla14
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

1.89 KB, patch
Justin Lebar (not reading bugmail)
: review+
Details | Diff | Splinter Review
(Assignee)

Description

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

Comment 1

6 years ago
Created attachment 605969 [details] [diff] [review]
Patch v1
Assignee: nobody → mounir
Status: NEW → ASSIGNED
Attachment #605969 - Flags: review?(jones.chris.g)
Perhaps you should use MOZ_ASSERT, which is an actual fatal (debug) assert?
(Assignee)

Comment 3

6 years ago
(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.
(Assignee)

Comment 4

6 years ago
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.
Attachment #605969 - Flags: review?(jones.chris.g) → review?(justin.lebar+bug)
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 on attachment 605969 [details] [diff] [review]
Patch v1

r-, since Chris and I seem to be in agreement.
Attachment #605969 - Flags: review?(justin.lebar+bug) → review-
(Assignee)

Comment 7

6 years ago
I think MOZ_ASSERT is a bit extreme. Can we find a compromise with NS_ASSERTION?
(Assignee)

Comment 8

6 years ago
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
Attachment #605969 - Attachment is obsolete: true
Attachment #606281 - Flags: review?(jones.chris.g)
(Assignee)

Comment 9

6 years ago
... with the patch that remove MOZ_ASSERT when mObservers is null.
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?
Attachment #606281 - Flags: review?(jones.chris.g)
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."
(Assignee)

Comment 12

6 years ago
(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.
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
(Assignee)

Comment 14

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

Using MOZ_ASSERT.
Attachment #606281 - Attachment is obsolete: true
Attachment #606600 - Flags: review?(jones.chris.g)
(Assignee)

Updated

6 years ago
Summary: Call NS_ERROR if RemoveObserver is called with an observer that is not present → Call MOZ_ASSERT if RemoveObserver is called with an observer that is not present
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.
Attachment #606600 - Flags: review?(jones.chris.g) → review-
(Assignee)

Comment 16

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

Oups...
Attachment #606600 - Attachment is obsolete: true
Attachment #606606 - Flags: review?(jones.chris.g)
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,/
Attachment #606606 - Flags: review?(jones.chris.g) → review+
Thanks jlebar!
https://hg.mozilla.org/mozilla-central/rev/0d61a0d8dba4
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
This patch should have had no effect on debug builds.  I'd be very surprised if it were the culprit.
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; }|.
(Assignee)

Comment 23

6 years ago
Would this be okay:
#ifndef DEBUG
  if (!mObservers) {
    return;
  }
#endif
I'd be happy with or without the ifndef.
https://hg.mozilla.org/mozilla-central/rev/436f102861fc
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.