53933 - xptiManifest.cpp leaks a file descriptor

Status: RESOLVED WONTFIX

(Core :: XPCOM, defect)

Description

[Jon Smirl]

line 242, fd is not freed.

    if(NS_FAILED(aFile->OpenNSPRFileDesc(PR_RDONLY, 0444, &fd)) || !fd)
        goto out;

    if(flen > PR_Read(fd, whole, flen))
        goto out;

 out:
    if(fd)
        PR_Close(fd);
    *pLength = flen;
    return whole;

Comment 1

[Jon Smirl]

It doesn't look like PR_Cleanup (which calls _PR_CleanupFdCache) is being 
called from the XPConnect standalone build.

I did greps and put a breakpoint on it.

Comment 2

[Jon Smirl]

Attachment: Add PR_Cleanup to the shutdown process

Comment 3

[Jon Smirl]

Attachment: Add PR_Cleanup to the shutdown process

Comment 4

[Jon Smirl]

Two attachments are identical, I had a problem with bugzilla.

The call to PR_Cleanup removes over 40 leaks from the simple case of 
start/shutdown XPCOM.

In nsThread.cpp:

void 
nsThread::Shutdown()
{
    if (gMainThread) {
        // XXX nspr doesn't seem to be calling the main thread's destructor
        // callback, so let's help it out:
        nsThread::Exit(NS_STATIC_CAST(nsThread*, gMainThread));
        nsrefcnt cnt;
        NS_RELEASE2(gMainThread, cnt);
        NS_WARN_IF_FALSE(cnt == 0, "Main thread being held past XPCOM 
shutdown.");
    }
}

nspr was not calling the thread's destructor because PR_Cleanup was not being 
called. I have commented out the call to nsThread::Shutdown() in 
XPCOM::Shutdown. nsThread::Shutdown down should probably be modified to remove:
        nsThread::Exit(NS_STATIC_CAST(nsThread*, gMainThread));
and still release gMainThread

Patch also enables a dump of leaked memory on debug builds under windows and 
MSVC. This will help in locating further leaks - there are several hundred.

Comment 5

[Jon Smirl]

Following two part patch is a more comprehensive fix. It needs bug 54264 and 
bug 53934 fixed to work.

It enables:
1) DLL unloading in component manager.
2) PR_Cleanup during XPCOM shut.

Fixing these two items eliminates approximately one hundred memory leaks.
It is only turned on for XPCOM standalone. It probably works for the browser 
too but I didn't test it.

Patch also enables a dump of leaked memory on debug builds under windows and 
MSVC. This will help in locating further leaks - there are several hundred.

Comment 6

[Jon Smirl]

Attachment: patch to nsThread is allow relase from PR_Cleanup

Comment 7

[Jon Smirl]

Attachment: Enables PR_Cleanup from XPCOM shutdown

Comment 8

[Asa Dotzler [:asa]]

Setting Jon Smirl's bugs to New

Comment 9

[Ray Whitmer]

Transferring to default XPCOM owner.

Comment 10

[dprice (gone)]

I'm adopting all of warren's leaks.

Comment 11

[dprice (gone)]

-> cathleen for a new home

Comment 12

[Matthias Versen [:Matti]]

-> cathleen

Comment 13

[Joel Nackman]

Jon,
I think this bug may be out of date.  I don't want to set it to INVALID because
I don't know enough C++ to properly check if the problem still exists.  However,
because line 207 of xptiManifest.cpp is a blank line, I doubt the problem still
exists.

It would be helpful if you emailed me and told me what the status of this bug is.

Thanks,
Joel Nackman 

Comment 14

[timeless]

afaik this bug isn't obsolete. i know i've played with calling pr_cleanup...

Comment 15

[Serge Gautherie (:sgautherie)]

The 2 patches have bit-rotted, especially the <nsThread.cpp> one.

Comment 16

[Benjamin Smedberg]

XPCOM is not going to call PR_Cleanup.