33697 - leaking mork stuff out of nsGlobalHistory::OpenDB()

Status: VERIFIED FIXED

(Core Graveyard :: History: Global, defect, P3)

Description

[Bruce Mitchener]

This happens on mozilla-bin startup/shutdown for me.  This afternoon's build,
Solaris 2.6, gcc 2.7.2.3, etc.

         MLK: 64 bytes leaked at 0xaed500
         This memory was allocated from:
               malloc         [rtlib.o]
               __bUiLtIn_nEw  [libxpcom.so]
               __builtin_new  [rtlib.o]
               orkinHeap::Alloc(nsIMdbEnv*,unsigned long,void**)
[orkinHeap.cpp:75]
               morkPool::NewHandle(morkEnv*,unsigned long,morkZone*)
[morkPool.cpp:176]
               morkEnv::NewHandle(unsigned long) [morkEnv.h:145]
               orkinFile::MakeFile(morkEnv*,morkFile*) [orkinFile.cpp:79]
                    MORK_ASSERT(isEnv);
                    if ( isEnv )
                    {
               =>     morkHandleFace* face = ev->NewHandle(sizeof(orkinFile));
                      if ( face )
                        return new(face) orkinFile(ev, face, ioObject);
                      else
               morkFile::AcquireFileHandle(morkEnv*) [morkFile.cpp:144]
                      f->AddStrongRef(ev->AsMdbEnv());
                    else // need new handle?
                    {
               =>     f = orkinFile::MakeFile(ev, this);
                      mObject_Handle = f;
                    }
                    if ( f )
               orkinFactory::OpenOldFile(nsIMdbEnv*,nsIMdbHeap*,const
char*,unsigned char,nsIMdbFile**) [orkinFactory.cpp:315]
                      morkFile* file = morkFile::OpenOldFile(ev, ioHeap,
inFilePath, inFrozen);
                      if ( file )
                      {
               =>       outFile = file->AcquireFileHandle(ev);
                        file->CutStrongRef(ev);
                      }
                        
               nsGlobalHistory::OpenDB() [nsGlobalHistory.cpp:1665]
                  
                      nsMdbPtr<nsIMdbFile> oldFile(mEnv); // ensures file is
released
                      err = factory->OpenOldFile(mEnv, dbHeap,
dbfile.GetNativePathCString(),
               =>                                dbFrozen,
getter_Acquires(oldFile));
                  
                      if ((err != 0) || !oldFile) return NS_ERROR_FAILURE;
                  
               nsGlobalHistory::Init() [nsGlobalHistory.cpp:1610]
               NS_NewGlobalHistory(nsISupports*,const nsID&,void**)
[nsGlobalHistory.cpp:533]
               nsGenericFactory::CreateInstance(nsISupports*,const nsID&,void**)
[nsGenericFactory.cpp:47]
               nsComponentManagerImpl::CreateInstance(const
nsID&,nsISupports*,const nsID&,void**) [nsComponentManager.cpp:1156]
               nsComponentManager::CreateInstance(const nsID&,nsISupports*,const
nsID&,void**) [nsRepository.cpp:81]
               nsServiceManagerImpl::GetService(const nsID&,const
nsID&,nsISupports**,nsIShutdownListener*) [nsServiceManager.cpp:293]
               nsServiceManagerImpl::GetService(const char*,const
nsID&,nsISupports**,nsIShutdownListener*) [nsServiceManager.cpp:431]
               nsServiceManager::GetService(const char*,const
nsID&,nsISupports**,nsIShutdownListener*) [nsServiceManager.cpp:544]
               nsGetServiceByProgID::operator ()(const nsID&,void**)const
[nsServiceManager.cpp:63]
               nsCOMPtr<nsIGlobalHistory>::assign_from_helper(const
nsCOMPtr_helper&,const nsID&) [nsCOMPtr.h:825]
               nsCOMPtr<nsIGlobalHistory>::nsCOMPtr<nsIGlobalHistory>(const
nsCOMPtr_helper&) [nsCOMPtr.h:527]
               nsDocShell::SetTitle(const unsigned short*) [nsDocShell.cpp:1538]
               nsHTMLDocument::SetTitle(const nsString&)
[nsHTMLDocument.cpp:796]
               HTMLContentSink::DidBuildModel(int) [nsHTMLContentSink.cpp:2285]
               CNavDTD::DidBuildModel(unsigned
int,int,nsIParser*,nsIContentSink*) [CNavDTD.cpp:641]
               nsParser::DidBuildModel(unsigned int) [nsParser.cpp:726]
               nsParser::ResumeParse(int,int) [nsParser.cpp:1214]
               nsParser::OnStopRequest(nsIChannel*,nsISupports*,unsigned
int,const unsigned short*) [nsParser.cpp:1652]
              
nsDocumentOpenInfo::OnStopRequest(nsIChannel*,nsISupports*,unsigned int,const
unsigned short*) [nsURILoader.cpp:276]
              
nsInputStreamChannel::OnStopRequest(nsIChannel*,nsISupports*,unsigned int,const
unsigned short*) [nsInputStreamChannel.cpp:366]

Comment 1

[Radha on family leave (not reading bugmail)]

Global history to waterson

Comment 2

[Chris Waterson]

david[b|mc]: is this the "mork was designed to leak" problem?

Comment 3

[David :Bienvenu]

I don't think files are built for leakage. but I could be wrong.

Comment 4

[Chris Waterson]

bruce: is nsGlobalHistory leaking? could you check the bloat logs? thanks.

Comment 5

[Bruce Mitchener]

To the best of my knowledge, it was the Mork environments which were designed to 
leak and I'm not reporting those in bug reports.  If I'm wrong in that, I'd love 
to know it too! (and I'll look to see if Global History leaks when my current 
run finishes)

Comment 6

[Bruce Mitchener]

XPCOM_MEM_LEAK_LOG says that nope, we aren't leaking nsGlobalHistory during 
mozilla-bin startup/shutdown.

Comment 7

[davidmc]

The store db returned from OpenOldFile() holds a reference to the file 
indefinitely until the store gets closed, which is done manually or when the 
store uses (ie strong refs) hit zero.  This might all be irrelevant.

It looks like the orkinFile instance gets leaked.  This is a 'handle' object 
holding a reference to the actual file.  I think I am allocating all the handles 
from a free list of such handles in the environment's pool, so this might be an 
instance of "mork envs are designed to leak" since the handle free lists would 
also be leaked.

However, when you are all done with a mork env, you can explicitly close it, 
which will free all memory being referenced by the env, and this would free any 
objects like free list handles.

So while env instances are designed to leak, we need not leak things referenced 
by an env since you can close them explicitly.

Comment 8

[Bruce Mitchener]

So, to test this, I can close mEnv in nsGlobalHistory::~nsGlobalHistory().  mEnv 
is of type nsIMdbEnv however, which doesn't export the call to 
morkEnv::CloseEnv().  morkEnv::CloseEnv() is called by morkEnv::CloseMorkNode() 
as well.  How do I get from an nsIMdbEnv to a morkEnv?

Comment 9

[davidmc]

One should call nsIMdbObject::CloseMdbObject() instead, since nsIMdbEnv is a 
subclass and all objects are closable this way.  Note this method perversely 
requires an instance of nsIMdbEnv as an argument.  I expect this might work if 
you passed the env to itself when closing, but I don't remember thinking about 
such a pathological case, so I'm a bit leery.  You could instead have around 
another env instance used only to close other env instances.  I know, it sounds 
silly, but it should be well-behaved. 

Comment 10

[Bruce Mitchener]

Thanks. I will try this out and test it under Purify and see what evil results 
from passing mEnv to mEnv->CloseMdbObject();

I will post results here.  Another neat trick would be to add in some Purify API 
calls to let Purify know about the memory handling with the free lists.  I don't 
have that type of energy this month or next though.

Comment 11

[Bruce Mitchener]

Placing a mEnv->CloseMdbObject(mEnv); in nsGlobalHistory::~nsGlobalHistory() 
didn't seem to free up any additional memory.  It didn't create any bad errors 
under Purify either though.  David: Do I need any CutStrongRef's or anything 
else to help make it go away?

Comment 12

[davidmc]

Hmm, you could put a breakpoint in morkEnv::CloseEnv() to see if you reach it, 
and then one in morkPool::ClosePool() to see if you reach that tool.  If you 
reach the former but not the latter, that would suggest the pool has more strong 
references to it (perhaps from a zone?).  I think it makes sense to add a line 
like the following to morkEnv::CloseEnv():

if ( mEnv_HandlePool ) mEnv_HandlePool->CloseMorkNode(ev);

early in the method.

Comment 13

[Chris Waterson]

bruce: how are we doing on this one? should i reassign to you?

Comment 14

[Bruce Mitchener]

I'll take it. I haven't had a lot of time, and probably won't until next week
though.

Comment 15

[Bruce Mitchener]

I'll accept this, but I won't have time to look at it for a long while.

Comment 16

[Bruce Mitchener]

This sounds like a nice project for some other genius.  I haven't any interest
in fixing leaks at the current time.

Comment 17

[Radha on family leave (not reading bugmail)]

goes back to waterson.

Comment 18

[Chris Waterson]

-> rjc

Comment 19

[Alec Flett]

Taking these history and history-related bugs.

Comment 20

[Viswanath Ramachandran]

nav triage team: not a beta stopper

Comment 21

[Alec Flett]

moving a few 0.9 bugs out to 1.0 to lighten my 0.9 load.

Comment 22

[Paul Chen]

nav triage team:

Marking nsbeta1+ and leaving at mozilla1.0

Comment 23

[Alec Flett]

argh.. take two at reassigning history bugs to new owner

Comment 24

[David :Bienvenu]

taking.

Comment 25

[David :Bienvenu]

I think this is fixed now.

Comment 26

[[e-mail] (gone - use [e-mail] instead)]

I'm assuming that Bruce's testcase refers only to the browser component, so this
is indeed fixed.  I ran Purify on Windows 2000 and mozilla.exe with no
arguments, and then just shutdown after http://www.mozilla.org loaded.  There
isn't a single trace of Mork in my output.

Verified FIXED.  (Note, we still have Mork leaks elsewhere, especially mail/news).