Closed Bug 33697 Opened 24 years ago Closed 23 years ago

leaking mork stuff out of nsGlobalHistory::OpenDB()

Categories

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

defect

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: bruce, Assigned: Bienvenu)

Details

(Keywords: memory-leak)

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]
Global history to waterson
Assignee: radha → waterson
david[b|mc]: is this the "mork was designed to leak" problem?
I don't think files are built for leakage. but I could be wrong.
bruce: is nsGlobalHistory leaking? could you check the bloat logs? thanks.
Keywords: mlk
Summary: MLK: mork-stuff out of nsGlobalHistory::OpenDB() → mork-stuff out of nsGlobalHistory::OpenDB()
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)
XPCOM_MEM_LEAK_LOG says that nope, we aren't leaking nsGlobalHistory during 
mozilla-bin startup/shutdown.
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.
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?
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. 
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.
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?
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.
bruce: how are we doing on this one? should i reassign to you?
I'll take it. I haven't had a lot of time, and probably won't until next week
though.
Assignee: waterson → bruce
Target Milestone: --- → M16
I'll accept this, but I won't have time to look at it for a long while.
Status: NEW → ASSIGNED
Target Milestone: M16 → M18
This sounds like a nice project for some other genius.  I haven't any interest
in fixing leaks at the current time.
Assignee: bruce → radha
Status: ASSIGNED → NEW
goes back to waterson.
Assignee: radha → waterson
Status: NEW → ASSIGNED
-> rjc
Assignee: waterson → rjc
Status: ASSIGNED → NEW
Target Milestone: M18 → Future
Taking these history and history-related bugs.
Assignee: rjc → alecf
Status: NEW → ASSIGNED
Summary: mork-stuff out of nsGlobalHistory::OpenDB() → leaking mork stuff out of nsGlobalHistory::OpenDB()
Target Milestone: Future → mozilla0.9
nav triage team: not a beta stopper
Keywords: nsbeta1-
Component: History: Session → History: Global
moving a few 0.9 bugs out to 1.0 to lighten my 0.9 load.
Target Milestone: mozilla0.9 → mozilla1.0
nav triage team:

Marking nsbeta1+ and leaving at mozilla1.0
Keywords: nsbeta1-nsbeta1+
Blocks: 92580
argh.. take two at reassigning history bugs to new owner
Assignee: alecf → blakeross
Status: ASSIGNED → NEW
Target Milestone: mozilla1.0 → ---
taking.
Assignee: blakeross → bienvenu
I think this is fixed now.
Status: NEW → RESOLVED
Closed: 23 years ago
QA Contact: claudius → stephend
Resolution: --- → FIXED
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). 
Status: RESOLVED → VERIFIED
No longer blocks: 92580
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.