18682 - Observer service should not hold strong references

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P1)

Description

[Suresh Duddi (gone)]

Currently the observerservice seems to hold a strong reference to the
nsIObservers thereby influencing their lifetime.

It should change to hold weak refs.

I dont think many are counting on it to maintain lifetime. Some stuff in
extensions/ like wallet and cookies might. We need to check before doing the
conversion.

Comment 1

[Scott Collins]

I think this is a very important bug.  The current bad ownership model is killing
us.  We need to fix this is a soon as possible.  Marking M12.

Comment 2

[Scott Collins]

The code for this is all checked in ...but I don't want to turn this facility

`on' until I fix the two remaining bugs.  Wallet observer service needs to hang

from something.  Someone is registering an observer that does not support weak

references.  If I turn this on before fixing these, the world becomes annoying

for users.  This two minimal bug fixes should be no problem during M12

stabilization.

Comment 3

[chris hofmann]

scott, what is the progress?

Comment 4

[Suresh Duddi (gone)]

Scott has fixes for this ready to go. I have reviewed it. We are waiting on your
approval.

Comment 5

[Suresh Duddi (gone)]

We will fix this for M13 after some renewed discussions.

Comment 6

[Warren Harris]

I see a checkin in nsWalletService.cpp line 51 that says:

1.43 <[e-mail]> 14 Dec 1999 10:51
 Make observers support weak references. Wallet service needs refcount
stabilization in constructor to avoid premature release. Bug #18682; r=dp.
a=dp.

This fix breaks the leak/bloat stats on tinderbox, making the nsTraceRefcnt
system think it's getting decremented to -1. It seems bogus to me that Init
should destroy the service prematurely. Let's find another way to do this.

Comment 7

[Warren Harris]

Here's what's happening: The way the NS_ADDREF macro (and nsTraceRefcnt) stuff
works is that it counts the first addref of an object as its construction and
increments the construction count. It knows it's the first addref by looking at
the refcnt. But in this case, the refcnt is already at 1, and it the first
NS_ADDREF gets called in the Init method incrementing it to 2, so it never
notices the construction. Then later it notices the destruction and says that
-1 objects of that class are still in existence.

What do we do?... I think calling Init from the constructor is bogus because
everything that it does can fail. If Init was was an interface method that was
called from elsewhere, 2 good things would happen: the errors from Init could
be detected, and the refcount stuff wouldn't get screwed up.

I'm not sure where Init should go though (perhaps in main1, nsAppRunner.cpp
line 604, before the WALLET_FetchFromNetCenter call).

Comment 8

[Scott Collins]

Well, this is all checked in and working.  Everything seems fine 2 weeks later.
Marking fixed.