Closed
Bug 735778
Opened 9 years ago
Closed 9 years ago
Call MOZ_ASSERT if RemoveObserver is called with an observer that is not present
Categories
(Core :: Hardware Abstraction Layer (HAL), defect)
Core
Hardware Abstraction Layer (HAL)
Tracking
()
RESOLVED
FIXED
mozilla14
People
(Reporter: mounir, Assigned: mounir)
References
Details
Attachments
(1 file, 3 obsolete files)
1.89 KB,
patch
|
justin.lebar+bug
:
review+
|
Details | Diff | Splinter Review |
Follow-up from bug 725951. So developers will know if they are doing something wrong with {Add,Remove}Observers.
Assignee | ||
Comment 1•9 years ago
|
||
Comment 2•9 years ago
|
||
Perhaps you should use MOZ_ASSERT, which is an actual fatal (debug) assert?
Assignee | ||
Comment 3•9 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•9 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 6•9 years ago
|
||
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•9 years ago
|
||
I think MOZ_ASSERT is a bit extreme. Can we find a compromise with NS_ASSERTION?
Assignee | ||
Comment 8•9 years ago
|
||
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•9 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)
Comment 11•9 years ago
|
||
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•9 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•9 years ago
|
||
Using MOZ_ASSERT.
Attachment #606281 -
Attachment is obsolete: true
Attachment #606600 -
Flags: review?(jones.chris.g)
Assignee | ||
Updated•9 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•9 years ago
|
||
Oups...
Attachment #606600 -
Attachment is obsolete: true
Attachment #606606 -
Flags: review?(jones.chris.g)
Comment 17•9 years ago
|
||
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!
Comment 19•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/0d61a0d8dba4
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
Comment 20•9 years ago
|
||
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 → ---
Comment 21•9 years ago
|
||
This patch should have had no effect on debug builds. I'd be very surprised if it were the culprit.
Comment 22•9 years ago
|
||
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•9 years ago
|
||
Would this be okay: #ifndef DEBUG if (!mObservers) { return; } #endif
Comment 24•9 years ago
|
||
I'd be happy with or without the ifndef.
Comment 25•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/436f102861fc
Status: REOPENED → RESOLVED
Closed: 9 years ago → 9 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•