Closed Bug 145712 Opened 18 years ago Closed 10 years ago

nsMsgRDFDataSource::Cleanup should not call nsIObserverService::removeObserver

Categories

(MailNews Core :: Backend, defect, trivial)

defect
Not set
trivial

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.3a1

People

(Reporter: timeless, Assigned: sgautherie)

Details

Attachments

(1 file)

 
This is a question, not a bug. File a bug when you know an answer to the question.
Status: UNCONFIRMED → RESOLVED
Closed: 18 years ago
Resolution: --- → INVALID
i spoke with alecf. we do want to change this.
Status: RESOLVED → UNCONFIRMED
Resolution: INVALID → ---
Summary: nsMsgRDFDataSource implements nsISupportsWeakReference do we want its ::Cleanup to unregister its observation? → nsMsgRDFDataSource::Cleanup should not call nsIObserverService::removeObserver
ok, but this is not my bug. Timeless, please at least take the time to explain,
for the record, why this is important, what impact it has, and so forth.
Assignee: alecf → mscott
QA Contact: gayatri → stephend
Valid, invalid?  I just want to get this into or out of the system, either way.

Timeless, can you elaborate?
 61 nsMsgRDFDataSource::~nsMsgRDFDataSource()
 62 {
 65     if (mInitialized) Cleanup();
 66 }

 72 nsMsgRDFDataSource::Init()
 73 {
 80     nsCOMPtr<nsIObserverService> obs =
do_GetService("@mozilla.org/observer-service;1",
 81                                                      &rv);
 83     rv = obs->AddObserver(NS_STATIC_CAST(nsIObserver*, this),
NS_XPCOM_SHUTDOWN_OBSERVER_ID, PR_TRUE);
 91 }

116 NS_INTERFACE_MAP_BEGIN(nsMsgRDFDataSource)
120     NS_INTERFACE_MAP_ENTRY(nsISupportsWeakReference)
122 NS_INTERFACE_MAP_END

 94 void nsMsgRDFDataSource::Cleanup()
 95 {
100     nsCOMPtr<nsIObserverService> obs =
do_GetService("@mozilla.org/observer-service;1",
101                                                      &rv);
103         rv = obs->RemoveObserver(NS_STATIC_CAST(nsIObserver*, this),
104                                  NS_XPCOM_SHUTDOWN_OBSERVER_ID);
111 }

ownsWeak  : If set to false, the nsIObserverService will hold a strong reference
to |anObserver|. If set to true and |anObserver| supports the nsIWeakReference
interface, a weak reference will be held. Otherwise an error will be returned.

Ok, so what happens?
1. We're a weak observer. This means that anyone who looks for us will be
willing to deal with the fact that we're dead. We're not supposed to waste
cycles telling them that we're dead.
2. If we weren't a weak observer then there'd be a strong reference to us which
would mean our destructor could never be called since that's the place where
we'd unregister ourselves (infinite unreachable loop)
Product: MailNews → Core
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P5
Assignee: mscott → nobody
OS: FreeBSD → All
Priority: P5 → --
QA Contact: stephend → backend
Hardware: PC → All
Assignee: nobody → sgautherie.bz
Status: NEW → ASSIGNED
Attachment #331247 - Flags: superreview?(bienvenu)
Attachment #331247 - Flags: review?(bienvenu)
Product: Core → MailNews Core
Attachment #331247 - Flags: superreview?(neil)
Attachment #331247 - Flags: superreview?(bienvenu)
Attachment #331247 - Flags: review?(neil)
Attachment #331247 - Flags: review?(bienvenu)
Comment on attachment 331247 [details] [diff] [review]
(Av1) <nsMsgRDFDataSource.cpp>
[Checkin: Comment 8]

Well I'm tempted to say that the destructor shouldn't be trying to clean up but I guess this is the safer fix.
Attachment #331247 - Flags: superreview?(neil)
Attachment #331247 - Flags: superreview+
Attachment #331247 - Flags: review?(neil)
Attachment #331247 - Flags: review+
Comment on attachment 331247 [details] [diff] [review]
(Av1) <nsMsgRDFDataSource.cpp>
[Checkin: Comment 8]


http://hg.mozilla.org/comm-central/rev/417083743e7e
Attachment #331247 - Attachment description: (Av1) <nsMsgRDFDataSource.cpp> → (Av1) <nsMsgRDFDataSource.cpp> [Checkin: Comment 8]
Status: ASSIGNED → RESOLVED
Closed: 18 years ago10 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.2a1
You need to log in before you can comment on or make changes to this bug.