Closed
Bug 594222
Opened 14 years ago
Closed 14 years ago
nsCategoryObsersver can attempt to remove its observers twice
Categories
(Core :: XPCOM, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: jdm, Assigned: jdm)
References
Details
Attachments
(1 file, 3 obsolete files)
2.17 KB,
patch
|
dougt
:
review+
dougt
:
approval2.0+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•14 years ago
|
||
Easy fix.
Assignee: nobody → josh
Attachment #472873 -
Flags: review?(benjamin)
Assignee | ||
Updated•14 years ago
|
Attachment #472873 -
Flags: review?(benjamin) → review?(doug.turner)
Comment 2•14 years ago
|
||
what was the assertion?
Assignee | ||
Comment 3•14 years ago
|
||
The ObserverList tries to get a weak reference from the observer object, and this asserts.
Comment 4•14 years ago
|
||
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?
Assignee | ||
Updated•14 years ago
|
Attachment #472873 -
Attachment is obsolete: true
Attachment #472873 -
Flags: review?(doug.turner)
Assignee | ||
Comment 5•14 years ago
|
||
Assignee | ||
Updated•14 years ago
|
Attachment #476635 -
Attachment is obsolete: true
Assignee | ||
Comment 6•14 years ago
|
||
This patch is better because it preserves the original behaviour of removing observers immediately when ListenerDied() is called.
Assignee | ||
Updated•14 years ago
|
Attachment #476647 -
Flags: review?(doug.turner)
Updated•14 years ago
|
Attachment #476647 -
Flags: review?(doug.turner) → review+
Assignee | ||
Comment 7•14 years ago
|
||
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?
Assignee | ||
Comment 8•14 years ago
|
||
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?
Assignee | ||
Comment 9•14 years ago
|
||
Here's a patch implementing the second option for your perusal.
Assignee | ||
Comment 10•14 years ago
|
||
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 11•14 years ago
|
||
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?
Assignee | ||
Comment 12•14 years ago
|
||
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.
Comment 13•14 years ago
|
||
resurrect it, and put it in my queue.
Assignee | ||
Updated•14 years ago
|
Attachment #472873 -
Attachment is obsolete: false
Attachment #472873 -
Flags: review?(doug.turner)
Assignee | ||
Updated•14 years ago
|
Attachment #480565 -
Attachment is obsolete: true
Updated•14 years ago
|
Attachment #472873 -
Flags: review?(doug.turner) → review+
Comment 14•14 years ago
|
||
Comment on attachment 472873 [details] [diff] [review] Patch low risk; xpcshell tests failures without this.
Attachment #472873 -
Flags: approval2.0+
Assignee | ||
Comment 15•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/e68dd8c5d9cd
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•14 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•