Closed Bug 594222 Opened 9 years ago Closed 9 years ago

nsCategoryObsersver can attempt to remove its observers twice

Categories

(Core :: XPCOM, defect)

x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: jdm, Assigned: jdm)

References

Details

Attachments

(1 file, 3 obsolete files)

While playing around with the patches in bug 508128, I came across an assertion while running the libpr0n xpcshell tests.  There is a race with xpcom shutdown and the nsCategoryCache such that if the observer sees xpcom-shutdown, it will happily remove its observers.  However, if at a later point the nsCategoryCache dies, it will call nsCategoryObserver->ListenerDied() which unconditionally attempts to remove the observers again, which ends up causing tests to fail when shutting down.
Attached patch PatchSplinter Review
Easy fix.
Assignee: nobody → josh
Attachment #472873 - Flags: review?(benjamin)
Attachment #472873 - Flags: review?(benjamin) → review?(doug.turner)
what was the assertion?
The ObserverList tries to get a weak reference from the observer object, and this asserts.
Comment on attachment 472873 [details] [diff] [review]
Patch

sorry this fell off my radar -- was suppose to be a fast review too...

can you use mListener instead of mObserversRemoved?
Attachment #472873 - Attachment is obsolete: true
Attachment #472873 - Flags: review?(doug.turner)
Attachment #476635 - Attachment is obsolete: true
This patch is better because it preserves the original behaviour of removing observers immediately when ListenerDied() is called.
Attachment #476647 - Flags: review?(doug.turner)
Attachment #476647 - Flags: review?(doug.turner) → review+
Comment on attachment 476647 [details] [diff] [review]
Stop nsCategoryObserver::RemoveObservers from being called after ListenerDied().

This change is quite safe and can prevent intermittent xpcshell test failures.
Attachment #476647 - Flags: approval2.0?
Comment on attachment 476647 [details] [diff] [review]
Stop nsCategoryObserver::RemoveObservers from being called after ListenerDied().

Lies. This patch doesn't do anything to improve the status quo, because we can still end up calling RemoveObservers a second time.  There are two choices: add a member variable that guards against multiple removal attempts and preverse the current eager removal behaviour, or move the contents of RemoveObservers to the xpcom-shutdown observer and don't bother trying to remove observers from ListenerDied.
Attachment #476647 - Attachment is obsolete: true
Attachment #476647 - Flags: approval2.0?
Here's a patch implementing the second option for your perusal.
Depends on: 587941
dougt: turns out we are seeing this in the wild as a cause of orange, so I'd like to get this in asap.  Could you help me decide on a strategy from comment 8?
Comment on attachment 480565 [details] [diff] [review]
Stop nsCategoryObserver::RemoveObservers from being called after ListenerDied().

hmm.  See bug 522353.  I think this basically reverts what they attempted to do.

On the patch that you commented on in comment 8, how can RemoveObservers be called twice?
The last patch calls RemoveObservers() before setting mListener to null, so the extra check makes no difference.  I think the easiest thing to do here is simply add the member variable, as in the original patch.
resurrect it, and put it in my queue.
Attachment #472873 - Attachment is obsolete: false
Attachment #472873 - Flags: review?(doug.turner)
Attachment #480565 - Attachment is obsolete: true
Attachment #472873 - Flags: review?(doug.turner) → review+
Comment on attachment 472873 [details] [diff] [review]
Patch

low risk; xpcshell tests failures without this.
Attachment #472873 - Flags: approval2.0+
http://hg.mozilla.org/mozilla-central/rev/e68dd8c5d9cd
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Blocks: 587941
No longer depends on: 587941
Duplicate of this bug: 532145
You need to log in before you can comment on or make changes to this bug.