Leaking nsEventQueueImpl objects

RESOLVED FIXED

Status

()

P3
normal
RESOLVED FIXED
19 years ago
18 years ago

People

(Reporter: beard, Assigned: pavlov)

Tracking

({memory-leak})

Trunk
memory-leak
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(11 attachments)

(Reporter)

Description

19 years ago
Looks like the final reference count is 2. I notice that you AddRef() in
the constructor. Somebody's not shutting you down. Here's the leaksoup summary
from the Boehm collector leak detector:

0x0476B368 [0] <nsEventQueueImpl> (32){120}
        0x066814A4
        0x066814FC
        0x00000002
        0x0476B2B0 [1] <void*> (28){88}
        0x00000001
        0x00000001
        0x00000000
        0x00000000
malloc
operator new(unsigned long, const std::nothrow_t&)
operator new(unsigned long)
nsEventQueueImpl::Create(nsISupports*, const nsID&, void**)
nsGenericFactory::CreateInstance(nsISupports*, const nsID&, void**)
nsComponentManagerImpl::CreateInstance(const nsID&, nsISupports*, const nsID&,
void**)
nsComponentManager::CreateInstance(const nsID&, nsISupports*, const nsID&, void**
)
EventQueueEntry::MakeNewQueue(nsIEventQueue**)
EventQueueEntry::EventQueueEntry(nsEventQueueServiceImpl*, ThreadKey&)
nsEventQueueServiceImpl::CreateEventQueue(PRThread*)
nsEventQueueServiceImpl::CreateThreadEventQueue()
nsCookieService::Init()
nsCookieService::nsCookieService()
nsCookieService::GetCookieService(nsICookieService**)
NS_NewCookieService(nsICookieService**)
CreateNewCookieService(nsISupports*, const nsID&, void**)
nsGenericFactory::CreateInstance(nsISupports*, const nsID&, void**)
nsComponentManagerImpl::CreateInstance(const nsID&, nsISupports*, const nsID&,
void**)
nsComponentManager::CreateInstance(const nsID&, nsISupports*, const nsID&, void**
)
nsServiceManagerImpl::GetService(const nsID&, const nsID&, nsISupports**,
nsIShutdownListener*)
nsServiceManager::GetService(const nsID&, const nsID&, nsISupports**,
nsIShutdownListener*)
nsGetServiceByCID::operator()(const nsID&, void**) const
nsCOMPtr<16nsICookieService>::assign_from_helper(const nsCOMPtr_helper&, const
nsID&)
nsCOMPtr<16nsICookieService>::nsCOMPtr<16nsICookieService>(const nsCOMPtr_helper&
)
nsViewerApp::SetupRegistry()
nsViewerApp::Initialize(int, char**)
main
__start
(Reporter)

Updated

19 years ago
Summary: Leaking nsEventQueueImpl objects → [MLK] Leaking nsEventQueueImpl objects

Updated

19 years ago
Status: NEW → ASSIGNED
Target Milestone: M14

Comment 1

19 years ago
Last time I looked at this, all event queues leaked because of other leaking objects holding
references to them. Generally it's only one object, and their refcount should consequently be 1.
I'll take a look.

Updated

19 years ago
Keywords: mlk

Updated

19 years ago
Target Milestone: M14 → M16

Updated

19 years ago
Summary: [MLK] Leaking nsEventQueueImpl objects → Leaking nsEventQueueImpl objects

Comment 2

19 years ago
Mass-moving all M16 non-feature bugs to M17, which we still consider to be 
part of beta2
Target Milestone: M16 → M17

Comment 3

19 years ago
moving to m18
Target Milestone: M17 → M18

Comment 4

19 years ago
mass-moving all bugs to m21 that are not dofood+, or nsbeta2+
Target Milestone: M18 → M21

Comment 5

19 years ago
*** Bug 41061 has been marked as a duplicate of this bug. ***

Comment 6

19 years ago
stealing
Assignee: danm → waterson
Status: ASSIGNED → NEW
(Assignee)

Comment 7

19 years ago
Created attachment 9425 [details] [diff] [review]
patch to reduce leaks a bunch
(Assignee)

Comment 8

19 years ago
so my patch reduces the number of refs a lot...

according to tinderbox, right now when we exit, we have 1 eventqueueimpl around
with 70 refs to it.

with this patch, we still have 1 eventqueueimpl around, but with only 4 refs to
it.

I know where a few of the places are that are leaking I believe and will try and
look at them.  This patch should greatly help with things that use event queues
through the event queue service more, like mailnews, modal dialogs, etc.

please let me know what problems n stuff you have and any suggestions you might
have on this patch.  The code is rather confusing, so let me know if you have
any questions.
(Assignee)

Comment 9

19 years ago
Created attachment 9426 [details] [diff] [review]
newer patch
(Assignee)

Comment 10

19 years ago
ok, heres a newer patch.  i think it cleans up after itself a little bit
better.  i don't exactly remember though.... alecf has been helping me a bit
trying to figure out the leaks... there is a comment in
EventQueueEntry::~EventQueueEntry() that points out at least one possible leak
and how to fix it... its just late so i didn't.  I think that is the only real
difference.
(Assignee)

Updated

19 years ago
OS: Mac System 8.6 → All
Hardware: Macintosh → All
(Assignee)

Comment 11

19 years ago
Created attachment 9455 [details] [diff] [review]
new patch (removes more cruft)
(Assignee)

Comment 12

19 years ago
it has been noted that dogfood+ bug 41124 is fixed with this patch
Blocks: 41124
(Assignee)

Comment 13

19 years ago
Created attachment 9467 [details] [diff] [review]
patch which fixes destruction of nsEventQueueServiceImpl
(Assignee)

Comment 14

19 years ago
Created attachment 9481 [details] [diff] [review]
latest revision
(Assignee)

Comment 15

19 years ago
Created attachment 9484 [details] [diff] [review]
newest patch (fixes all leaks so long as the EventQueueService is shutdown)
(Assignee)

Comment 16

19 years ago
Created attachment 9531 [details] [diff] [review]
newest patch (propsed final fix) (w/o mailnews changes)
(Assignee)

Comment 17

19 years ago
I just attached the newest patch. (It shouldn't be much different from the
previos patch I posted.)  This patch doesn't contain any changes to mailnews. 
Alec Flett has got some patches which fix the mailnews leaks which were in turn
holding on to things that leaked the EventQueueService.  I will either attach
these or have him attach these as soon as I can get ahold of him.

This latest patch is my proposed fix for this bug.  I would like people to
carefully review it and see if I have missed anything.

I will go through and make another patch sometime today to correct some old
comments about how certain things work (there are a few large comments that are
not accurate anymore).
(Assignee)

Comment 18

19 years ago
Created attachment 9553 [details] [diff] [review]
newest patch.  fixes assertion on mac and linux

Comment 19

19 years ago
Created attachment 9554 [details]
attaching image as a test

Comment 20

19 years ago
on a related note, I just checked in the IMAP weakref stuff, so IMAP should be 
safe now.

Comment 21

19 years ago
Created attachment 9555 [details]
another test attachment
(Assignee)

Comment 22

19 years ago
Created attachment 9609 [details] [diff] [review]
hopefully final patch
(Assignee)

Comment 23

19 years ago
ok, I just attached what is hopefully the final patch.  Waterson has reviewed it
and I have changed the 1 thing he mentioned.  He says it looks good to him.  It
has been tested on Windows, Mac, and linux.  I will get danm to review the patch
tomorrow and will hopefully land it tomorrow evening.
Assignee: waterson → pavlov

Comment 24

18 years ago
r=waterson
Reading the code finally, found this problemo, in 
xpcom/proxy/src/nsProxyEventObject.cpp:

     // this will be our key in the hash table.  
     // this must not be a nsCOMPtr since we need to make sure that we do a QI.
     nsCOMPtr<nsISupports> destQRoot;
-       if(NS_FAILED(destQueue->QueryInterface(NS_GET_IID(nsISupports), 
(void**)&destQueue)))
-    return nsnull;
+    // XXX what is this here for? (pav)
+    // if(NS_FAILED(destQueue->QueryInterface(NS_GET_IID(nsISupports), 
(void**)&destQueue)))
+    //    return nsnull;
 
-
+    // destQRoot is always null.... is this on purpose?
     char* rootKeyString = PR_sprintf_append(nsnull, "%p.%p.%d", 
(PRUint32)rootObject.get(), (PRUint32)destQRoot.get(), proxyType);
     nsStringKey rootkey(rootKeyString);

Clearly there was a typo, or brain-fart: the destQRoot variable should have been 
set by the QI to nsISupports.  That's necessary for COM identity: the "root" of 
any object, however much aggregation is going on, is the pointer you get when 
you QI to ISupports.  So XXX ... (pav) comments should be removed, restoring the 
QI, but the QI should set destQRoot:

    nsresult rv;
    nsCOMPtr<nsISupports> destQRoot = do_QueryInterface(destQueue, &rv);
    if (NS_FAILED(rv))
        return nsnull;

Make sense?

Who did a number on the bracing and indentation style here?  I see hacked-in 
if-else and if statements that use two-space indentation and travis(?)-style 
bracing (oh the horror).  Fix it if you can.

/be
(Assignee)

Comment 26

18 years ago
fix checked in.
Status: NEW → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.