nsCategoryObsersver can attempt to remove its observers twice

RESOLVED FIXED

Status

()

Core
XPCOM
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: jdm, Assigned: jdm)

Tracking

Trunk
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

8 years ago
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

8 years ago
Created attachment 472873 [details] [diff] [review]
Patch

Easy fix.
Assignee: nobody → josh
Attachment #472873 - Flags: review?(benjamin)
(Assignee)

Updated

8 years ago
Attachment #472873 - Flags: review?(benjamin) → review?(doug.turner)

Comment 2

8 years ago
what was the assertion?
(Assignee)

Comment 3

8 years ago
The ObserverList tries to get a weak reference from the observer object, and this asserts.

Comment 4

8 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

8 years ago
Attachment #472873 - Attachment is obsolete: true
Attachment #472873 - Flags: review?(doug.turner)
(Assignee)

Comment 5

8 years ago
Created attachment 476635 [details] [diff] [review]
Stop nsCategoryObserver::RemoveObservers from being called after ListenerDied().
(Assignee)

Updated

8 years ago
Attachment #476635 - Attachment is obsolete: true
(Assignee)

Comment 6

8 years ago
Created attachment 476647 [details] [diff] [review]
Stop nsCategoryObserver::RemoveObservers from being called after ListenerDied().

This patch is better because it preserves the original behaviour of removing observers immediately when ListenerDied() is called.
(Assignee)

Updated

8 years ago
Attachment #476647 - Flags: review?(doug.turner)

Updated

8 years ago
Attachment #476647 - Flags: review?(doug.turner) → review+
(Assignee)

Comment 7

8 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

8 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

8 years ago
Created attachment 480565 [details] [diff] [review]
Stop nsCategoryObserver::RemoveObservers from being called after ListenerDied().

Here's a patch implementing the second option for your perusal.
(Assignee)

Updated

8 years ago
Depends on: 587941
(Assignee)

Comment 10

8 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 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

8 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.
resurrect it, and put it in my queue.
(Assignee)

Updated

8 years ago
Attachment #472873 - Attachment is obsolete: false
Attachment #472873 - Flags: review?(doug.turner)
(Assignee)

Updated

8 years ago
Attachment #480565 - Attachment is obsolete: true

Updated

8 years ago
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+
(Assignee)

Comment 15

7 years ago
http://hg.mozilla.org/mozilla-central/rev/e68dd8c5d9cd
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
(Assignee)

Updated

7 years ago
Blocks: 587941
No longer depends on: 587941
(Assignee)

Updated

7 years ago
Duplicate of this bug: 532145
You need to log in before you can comment on or make changes to this bug.