Closed Bug 15598 Opened 25 years ago Closed 23 years ago

Mork leaks environments

Categories

(MailNews Core :: Database, defect, P2)

Product:
MailNews Core
Component:
Database
Platform:
All
Windows NT
Type:
defect

Tracking

(Not tracked)

Status:
VERIFIED FIXED

People

(Reporter: Bienvenu, Assigned: Bienvenu)

References

Details

(Keywords: memory-leak) 

The fact that Mork leaks environments blocks the memory leak team from getting
to 0 memory leaks. This needs to be fixed somehow.
QA Contact: rickg → leger
BTW, we should have a bugzilla category for MORK. I just co-opted xp utilities
for now. CC'ing Jan Leger.
From http://bugzilla.mozilla.org/show_bug.cgi?id=15598:

David Bienvenu writes:
> The fact that Mork leaks environments blocks the memory leak team
> from getting to 0 memory leaks. This needs to be fixed somehow.

I was highly entertained to see the space team had the goal of getting
to zero leaks, since Mork is designed to leak environments, factories,
and heaps on purpose.  The big one is environments, of course.  There
need not be many, of course, but I think we currently make more than
are necessary; since they get leaked, they should go on free lists.

The big reason for this design was because XPCOM style addref() and
release() methods stupidly do not take any parameters, so it is not
possible to pass a context parameter to Mork without having every
single handle object hold a reference to an environment instance.

There are several approaches to different reasonable solutions.
First, the space team can give up on the goal of zero leaks, since
they should have considered the possibility that some designs will
leak on purpose.  Accounting for this, they might forget about it.

Second, we can remove the addref() and release() methods which were
added to mimic the XPCOM refcounting, and then modify the MDB callers
so they use AddStrongRef() and CutStrongRef() as appropriate, making
them responsible for passing an environment as orginally designed.
This sounds fine to me.

Third, we can change the Mork implementation so it does massively
more ref-counting of environment objects, since Mork is forced to
put one into every object returned from the MDB interface.  The
goal would be to refcount perfectly so envs go away as desired.  I
do not support this choice because it is going to fail, because
there are too many opportunities for entropy when we add hundreds
of thousands of additional refcounting calls for cheap throw-away
objects that might either slip through cracks, or get whacked. I
think this choice will make Mork have larger intermittent leaks that
will never come under control, and degrade the Mork runtime quality.

Fourth, we could try massively redesigning Mork so environments are
not needed when releasing references; this is actually so much work
that it seems completely infeasible.

The easiest thing to do is have the space team stop counting Mork
objects that get leaked by design.  The second easiest thing is to
fix the problematic addref() and release() methods by removing them.
The other choices are depressing and asking for lots of trouble.

David Mc
I think all the current environment releases use CutStrongRef, but we still seem
to leak environments. All we need is a call to free all environments. It can be
called from the shutdown method of the mork component. If needs be, I can do
this myself.
Because they're designed to be leaked, I deliberately bump the refcount
artificially high when the environments are created.  I forget, but the
constant might contain the substring "Slop" as part of the name.  One
might try removing that to see what happens.
Another possibility, which is a hack for the specific purpose of satisfying this
spurious metric used by the space team, is to keep all envs on a list, and
destroy them all when some function is called.  Then this could be called when
the space team runs their tests, but not otherwise under real world conditions.
Yes, that's what I had in mind by suggesting a call to free all allocated
environments. I'm trying the approach of getting rid of the ref-counting slop
first, however.
This comment is made rather blindly by not knowing anything about the
implementation details of your environments, so take it with a *large* grain of
salt.

Another option not listed by david is to use arena's to hold the storage of the
environments and then free the arena in well fell swoop. Especially works well
when the lifetimes of the sub-objects are well controlled.

Of course you probably already know that...

All we are asking for is that during shutdown the libraries release any globally
held storage so that we can discover more easily where the true leaks are.
remove the pattern |mNode_Refs += morkEnv_kWeakRefCountEnvBonus;| from
environment constructors to kill the slop factor.

The MDB interface already supports arena usage on purpose through the interface,
by permitting subclasses of nsIMdbHeap to be used for storage allocation.  But
currently callers pass nil which causes a default heap implementation to be used
instead, probably based on ::operator new() etc.

The new morkZone class is basically an arena, and it's a subclass of nsIMdbHeap,
so that might make a good choice if we wanted to go that way.  Probably would
need to reduce the size of hunk blocks allocated though, so the global arena zone
did not gratuitously allocate orders of magnitude more space than really needed.
thanks, I found the Bonus stuff and changed it. I'm still poking around, though.
We still leak a bunch of stuff.
Um, but the zone would need to have destruction examined to see if it actually
used the environment passed when a zone is closed.  Basically the problem is that
environments are the thing upon which everything in Mork depends.  The method
that makes an environment instance is about the only method that does not take an
environment parameter.  Assuming that environments will leak has a simplifying
effect on shutting down a Mork system, since it avoid circular dependencies on
having a well-behaved environment to continue closing things.
Although environments are intended to be leaked, they can be manually closed by a
call to nsIMdbObject::CloseMdbObject() in order to free all resources even when
leakage is planned.  But this implies one intends to keep at least one env in
hand, so it can be passed as the env parameter when other env instances are
closed.  Isn't meta-completeness fun? :-)
One might ask, "So what's with the twitchy environment things anyway?"  They are
intended to replace effects that would otherwise require global variables, so
that Mork can avoid globals and be generally reentrant.  Globals are a different
twitchy problem, worse than the explicitly crafted environment twitchy problem.
My inclination is to try to short circuit all this stuff in
orkinEnv::CutStrongRef - if the ref count is 0, clean up the environment. Is
this doomed to failure?
Mork separates strong refs (released by CutStrongRef()) from weak refs (released
by CutWeakRef()), and the former cooperatively controls closing, while the latter
cooperatively controls deallocation.  Cleanup should already happen when refs hit
zero; closing on zero strong refs, freeing on zero weak refs.  If you find
yourself wanting to do freeing violence when strong refs hit zero, then you are
making the weak ref system invalid.  Maybe there's a weak ref leak;  you could
look for any extant calls to morkEnv::SlotWeakEnv() or morkEnv::SlotStrongEnv().
My best suggestion is to use an NSPR area to implement a subclass of this
heap interface below, where only Alloc() need to anything, and all the
other methods can do nothing at all.  Then pass this heap to methods that
need one, like nsIMdbFactory::MakeEnv().  They you can blow away everything
in one fell swoop later when you're ready to kill the arena.

I might need to double check places where folks are defaulting to use of
the environment's heap for allocation, so we avoid using the arean globally
for *everything*, in case the current heap is best most of the time.

class nsIMdbHeap { // caller-supplied memory management interface
public:
// { ===== begin nsIMdbHeap methods =====
  virtual mdb_err Alloc(nsIMdbEnv* ev, // allocate a piece of memory
    mdb_size inSize,        // requested byte size of new memory block
    void** outBlock) = 0;   // memory block of inSize bytes, or nil

  virtual mdb_err Free(nsIMdbEnv* ev, // free block from Alloc or Resize()
    void* ioBlock) = 0;     // block to be destroyed/deallocated

  virtual mdb_err HeapAddStrongRef(nsIMdbEnv* ev) = 0;
  virtual mdb_err HeapCutStrongRef(nsIMdbEnv* ev) = 0;

// } ===== end nsIMdbHeap methods =====
};
No real cleanup happens because environments put themselves back in the handle
pool when their count goes to 0. That handle pool never gets cleaned up.
The orkinEnv handle instances should go in the pool, but the morkEnv instances
should be cleaned up.  That pool never gets cleaned up, because the heap used is
the one that's an integral part of the factory instance; and that heap gets used
because a nil pointer gets passed from callers to MakeEnv(), and this causes the
factory to use the internal heap member.   So the pool would be expected to be
cleaned up only if the factory is closed, or if an explicitly created and passed
heap instance is used instead of nil for MakeEnv().
So the pool will probably be taken care of by making an arena based subclass of
nsIMdbHeap.  Or we can make it possible to close a factory without passing an env
instance -- I can carefully make that work by using an internal env member in the
factory that doesn't get closed until everything else is closed.

Also, we might consider changing the way in which an initial factory instance is
made so that a heap instance can be passed; maybe this should be an extra way to
make factories.  Then both factory and environments, handles, etc would get blown
away by the arena when the mailnews world is shut down.
I tried explicitly closing the factory and environment objects, and that helped
some. Now, I'm going to try the nsIMdbHeap subclass with an NSPR arena thingy
for the folder cache and see what happens.  But first, I need a triple latte.
Blocks: 16654
QA Contact: leger → rickg
Resetting QA contact from leger.
Target Milestone: M14
Keywords: mlk
Summary: [MLK] Mork leaks environments → Mork leaks environments
these are mine now, I guess.
Assignee: davidmc → bienvenu
accepting
Status: NEW → ASSIGNED
m20
Target Milestone: M14 → M20
*** Bug 50199 has been marked as a duplicate of this bug. ***
*** Bug 50199 has been marked as a duplicate of this bug. ***
Keywords: mail1
changing priorities
Priority: P3 → P2
David is this leaking a lot of memory?
No. The issue with this and other small mork leaks is that they show up in the
leak logs, and confuse the issue. It's like compiler warnings. Any given warning
is most likely not serious. but the non-serious ones make it harder to find the
serious ones.
marking nsbeta1-.  If we start doing memory leak work and we need to fix this,
we can ignore the nsbeta1-.  But it's not worth fixing this unless we are doing
other leak work.
Keywords: nsbeta1-
Blocks: 92580
No longer blocks: 92580
should be fixed now.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
I haven't seen this in doing:

IMAP logins
folder switching
reading of mail
switching newsgroups
downloading newsgroup headers
deleting messages

Verified FIXED.
Status: RESOLVED → VERIFIED
Component: XP Utilities → Mail Database
Product: Browser → MailNews
Product: MailNews → Core
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.