Closed Bug 735778 Opened 8 years ago Closed 8 years ago

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

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla14

People

(Reporter: mounir, Assigned: mounir)

References

Details

Attachments

(1 file, 3 obsolete files)

Follow-up from bug 725951. So developers will know if they are doing something wrong with {Add,Remove}Observers.
Attached patch Patch v1 (obsolete) — Splinter Review
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?
(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 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-
I think MOZ_ASSERT is a bit extreme. Can we find a compromise with NS_ASSERTION?
Attached patch Patch v2 (obsolete) — Splinter Review
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)
... 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."
(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
Attached patch Patch v3.1 (obsolete) — Splinter Review
Using MOZ_ASSERT.
Attachment #606281 - Attachment is obsolete: true
Attachment #606600 - Flags: review?(jones.chris.g)
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-
Attached patch Patch v3.1Splinter Review
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+
https://hg.mozilla.org/mozilla-central/rev/0d61a0d8dba4
Status: ASSIGNED → RESOLVED
Closed: 8 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; }|.
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
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.