Created attachment 596003 [details] [diff] [review] 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.
Why was nsScreen trying to remove an observer that didn't exist?
(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.
What's nsScreen::Invalidate()? Why does it need to remove itself from the observer list?
(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.
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 on attachment 596003 [details] [diff] [review] Patch Getting this out of my review queue until we finish the background discussion.
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 on attachment 596003 [details] [diff] [review] Patch Mounir asked me to review this for him.
Attachment #596003 - Flags: review?(jones.chris.g) → review?(justin.lebar+bug)
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 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.)
Attachment #596003 - Flags: review?(justin.lebar+bug) → review+
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Whiteboard: [needs review]
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 → ---
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago → 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.