110850 - XPComify public mdb interfaces, and remove orkin layer from Mork

Status: VERIFIED FIXED

(MailNews Core :: Database, defect)

Description

[David :Bienvenu]

This bug is to track the work of making Mdb+Mork support XPCOM, and get rid of
orkin layer of Mork. The orkin objects only reasons for being were to support
xpcom ref-counting semantics without requiring the caller to pass in an
environment, and to implement the MDB interfaces.  The change is to make the
underlying mork objects directly implement MDB interfaces, and to inherit from
nsISupports instead of nsIMdbSupports.  Most of the work is in Mork, but there
are some changes in mailnews/base and mailnews/db, and xpfe/components/history
(because CloseMdbObject goes away).
This fix should reduce some memory bloat, since we won't need the orkin objects,
and it fixes a lot of memory leaks because the mork strong and weak ref-counting
semantics made it so that many objects were never released. The code should be
easier to maintain as well, now that an extra layer is gone.

Comment 1

[David :Bienvenu]

I'll start attaching patches now. I've been running with this for several days
without a problem (under purify as well) but I need to go over the code some
more to make sure I haven't broken anything.

Comment 2

[David :Bienvenu]

Attachment: changes to mdb, the public interface file

Most of these changes are to use NS_IMETHOD explicitly, and inherit from
nsISupports. I've also removed some unused interfaces. I'd appreciate it if you
guys could start reviewing because it's going to take a while :-(

Comment 3

[David :Bienvenu]

Attachment: remove unused mork files.

A couple of these have never been used, but most of the orkin ones will become
unused when the rest of the changes get checked in.

Comment 4

[David :Bienvenu]

Attachment: changes to go along with mork changes

Basically, CloseMdbObject/CutStrongRef(mEnv) -> Release

Comment 5

[David :Bienvenu]

Alec, would you r or sr the history changes? Thx.

Comment 6

[David :Bienvenu]

Attachment: corresponding changes for folder cache.

Use Release() directly.

Comment 7

[Alec Flett]

Comment on attachment 58440 [details] [diff] [review]
changes to go along with mork changes

bienvenu you are a monster!
sr=alecf

Comment 8

[Navin Gupta]

Comment on attachment 58437 [details] [diff] [review]
changes to mdb, the public interface file

r=naving

Comment 9

[Navin Gupta]

Comment on attachment 58439 [details] [diff] [review]
remove unused mork files.

r=naving

Comment 10

[Navin Gupta]

Comment on attachment 58440 [details] [diff] [review]
changes to go along with mork changes

r=naving

Comment 11

[Navin Gupta]

Comment on attachment 58441 [details] [diff] [review]
corresponding changes for folder cache.

r=naving

Comment 12

[Navin Gupta]

Good job david, that was easy to review. So do you see the size of mork.dll go
up or down and by what.

Currently mork.dll has size = 683736 bytes. This is from purify opt build (a few 
days old). 

Comment 13

[David :Bienvenu]

Attachment: mork diffs to get rid of orkin layer

OK, this is the hard part. I took all the orkin methods and moved them into
their mork counterparts, and changed them to work as methods of the mork
classes. Sorry for the volume of the changes :-(

Comment 14

[David :Bienvenu]

Attachment: address book diffs

coresponding address book diffs, plus changes to force closed any open address
books at shutdown.

Comment 15

[David :Bienvenu]

I don't know how much smaller this will make an optimized build of mork.dll - I
would expect a small decrease (maybe 3-5%) but that's just a guess. I removed a
few unused classes, but most of the code was just moved from ork to mork. I
expect the runtime savings to be in the same range, but the big change will be
the reduction of leaks when switching between folders, which will cut down the
bloat numbers quite a bit, I believe.

Comment 16

[David :Bienvenu]

Attachment: msgdb changes

msg db changes, mostly just using AddRef and Release

Comment 17

[Navin Gupta]

Comment on attachment 58500 [details] [diff] [review]
mork diffs to get rid of orkin layer

Could you explain why you changed size here ?
-  inSize += 8; // sizeof(mork_u4) * 2
+  inSize += 12; // sizeof(mork_u4) * 3


+mork_refs
+morkTable::AddStrongRef(morkEnv *ev)
+{
+  return (mork_refs) AddRef();
+}
+
how is *ev used here? could you explain more 

+mork_refs
+morkTable::CutStrongRef(morkEnv *ev)
+{
+  return (mork_refs) Release();
+}

when moving methods from 
orkin layer you chose not
to implement certain methods, why did you do
that ?

Comment 18

[David :Bienvenu]

the change in size has to do with how I had to tweak the HEAP_STATS debugging
code (the code you're asking about is not turned on by default, either in debug
or release builds, if that helps). I needed to add the heap object to the
allocated blocks so that when I freed them, I could know which heap they were
getting freed from and adjust the various heap stats debug counts for that heap.

The ev is not used, but when you're overriding a virtual method like
AddStrongRef, you need to have the same argument list as the overridden virtual
method. Why did I have to override those methods? I had to do that so that that
object used xpcom ref-counting semantics instead of the mork ref-counting
semantics. I couldn't change the call site because it's called from a
polymorphic method that gets called on lots of internal objects. Clear as mud,
right? :-(

Comment 19

[Navin Gupta]

Comment on attachment 58533 [details] [diff] [review]
address book diffs

r=naving

you can remove null 
assignments after release

+      m_mdbEnv->Release();
       m_mdbEnv = nsnull;


+		m_mdbStore->Release();
		m_mdbStore = nsnull;

Comment 20

[David :Bienvenu]

I could remove the = null in the destructor, but I definitely can't remove the
second one with the store!

Comment 21

[Navin Gupta]

Comment on attachment 58596 [details] [diff] [review]
msgdb changes

r=naving
you can remove null assignments after release

Comment 22

[Navin Gupta]

Comment on attachment 58500 [details] [diff] [review]
mork diffs to get rid of orkin layer

r=naving

thanks for explaining it.

Could you double check for
the methods that were not 
implemented.

Comment 23

[David :Bienvenu]

thanks for reviewing it. I'm pretty confident the unimplemented methods are
correct and not called because I would see assertions.

Comment 24

[(not reading, please use [e-mail] instead)]

sr=sspitzer on all patches.

Comment 25

[David :Bienvenu]

fixes checked in.

Comment 26

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

This landed, and I'll be verifying the separate orkin leaks...

Verified FIXED.

Comment 27

[Roland Mainz]

bienvenu, wanna take a look at this, please ?

From "nebiros" tinderbox:
-- snip --
"/usr/include/memory.h", line 21: Error: Only one of a set of overloaded
functions can be extern "C".
"/usr/include/memory.h", line 21: Error: std::memchr(const void*, int, unsigned
long), returning void*, was previously declared returning const void*.
gmake[5]: *** [orkinHeap.o] Error 2
gmake[5]: Leaving directory
`/builds/tinderbox/SeaMonkey/SunOS_5.7_Depend/mozilla/obj-sparc-sun-solaris2.7/db/mork/src'
gmake[4]: *** [libs] Error 2
-- snip --

Comment 28

[David :Bienvenu]

I have been looking, and I have no idea what's going on. If you have any idea
what's going on, let me know.

Comment 29

[Roland Mainz]

I filed bug 111574 ("WS6U2 tinderbox burning due broken
db/mork/src/orkinHeap.cpp") for that issue ...