56584 - ns4xPlugin::CreatePlugin leaks ref to nsMemoryImpl

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect, P3)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The reference ns4xPlugin::CreatePlugin aquires to the global nsMemoryImpl
through a GetService call seems to be leaked in every run of Mozilla.  I'm not
sure where it should be released or why it isn't.

One possibility is that ns4xPlugin::CheckClassInitialized is setting mMalloc to
null without releasing it.  But that may or may not be it.

Comment 1

[ekrock's old account (dead)]

Not a Netscape 6 RTM blocker. FUTURE. This bug has been marked Future because
the Netscape engineer it is assigned to is overburdened.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The problem is that ns4xPlugin::Shutdown is never called, so mMalloc is never
freed.  But it really seems wrong to release a static variable every time
Shutdown is called, although I'm not sure when it's supposed to be called.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch that fixes leak

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The above patch fixes the leak, but I'm really not sure what the correct fix is.
 What's supposed to happen?  Why is mMalloc set to null in
CheckClassInitialized, and why is it currently released in Shutdown?

Comment 5

[av (gone)]

Please look at the code after the fix for bug 58491 got in. It became global and 
not static, but some questions still remain, and I think we still should go with 
releasing it in the destructor.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: revised patch

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I think it should be static so that there aren't name conflicts with other files.

I don't see why either gServMgr or gMalloc was set to null in
CheckClassInitialized, so I took those lines out rather than changing them the
latter to a release.  Why were they there?

Comment 8

[Chris Waterson]

The last patch looks ok to me. av, what do you think?

Comment 9

[av (gone)]

David, I don't know what the intention of the person who nulled gServMgr 
and gMalloc was, but I too don't see any reason for doing that. Your patch looks 
good and clean to me. r=av or a=av whatever you need to get it in.

Comment 10

[Brendan Eich [:brendan]]

Doesn't C++ default to static scope and lifetime (file scope) unless there is an
extern on the definition as well as on any declarations?

/be

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I'm not sure, but I believe IRIX's compiler doesn't, which is a good reason to
say |static| explicitly.

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

From reading C++ section 3.5 paragraphs 3-4, I think objects have internal
linkage only when
 * explicitly declared |static|, or
 * when declared |const| and not |extern|

Comment 13

[Chris Waterson]

sr=waterson

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Fix checked in 2000-12-08 20:06 PST.

Comment 15

[shrirang khanzode]

verified that the patch is in. (rubberstamp) VERIFIED.