[prev in list] [next in list] [prev in thread] [next in thread]
List: kmail-devel
Subject: PATCH: fix memory leak and Re: Zero-copy parsing of messages achieved
From: Don Sanders <sanders () kde ! org>
Date: 2002-09-02 12:50:13
[Download RAW message or body]
On Monday 02 September 2002 15:02, Karl-Heinz Zimmer wrote:
> On Monday 02 September 2002 01:58, Don Sanders wrote:
> > On Sunday 01 September 2002 19:18, Karl-Heinz Zimmer wrote:
>
> (...)
>
> > > If not I should do some *serious* optimizations on the partNode
> > > class!
> >
> > What would you like to optimize?
>
> Some time ago I was told by KMail hackers that the partNode class
> is not a very smart thing so I allways had the feeling there must
> be several things that I had overlooked when designing it but
> (since I had not expected it to be still there in KDE 3.1) I did
> not take to time to contact them again for further information and
> look for ways how to improve this class...
I don't mean to make an overwhelming commendation of partNode. But the
fact that partNode uses a dwString instead of some other string or
array type means that it can be and is memory efficient. I was
pleased that I didn't have to spend time working on partNode to make
it more efficient. Unlike KMMessagePart whcih is not dwString based
and as a result will require quite some work to be made efficient.
That's why I said thanks because working with partNode and
KMMessagePart made it obvious to me that message classes should be
dwString based rather than QByteArray based. At least until
QByteArray supports substring sharing, but I'm pretty sure there are
currently no plans for that.
> (...)
>
> > > // store message body in mRootNode if *no* body parts found
> > > // (please read the comment below before crying about me)
> > > :-) DwBodyPart* mainBody = 0;
> > > DwBodyPart* firstBodyPart = aMsg->getFirstDwBodyPart();
> > > if( !firstBodyPart ) {
> > > // ATTENTION: This definitely /should/ be optimized.
> > > // Copying the message text into a new body part
> > > // surely is not the most efficient way to go.
> > > // I decided to do so for being able to get a
> > > // solution working for old style (== non MIME)
> > > // mails without spending much time on
> > > implementing. // During code revisal when switching
> > > to KMime // all this will probably disappear anyway
> > > (or it // will be optimized, resp.). (khz,
> > > 6.12.2001) kdDebug() << "*no* first body part found, creating
> > > one from Message" << endl;
> > > mainBody = new DwBodyPart(aMsg->asDwString(), 0);
> > > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > > mainBody->Parse();
> > > }
> > >
> > > The marked line results in old style messages without
> > > Content-Type header being copied entirely into a new DwString.
> > > :-(
> >
> > Are you sure memory copying occurs here? DwString is shared.
>
> I am not sure.
> Depending on the answer we get regarding to KMime I will or won't
> look for this before the KDE 3.2 release...
I strongly suspect your comment is totally misleading. I think it's
not memory ineffiicent as the dwString is shared.
However funnily enough after spending several hours hunting down a
memory leak I ended up at that line of code. Every time a message is
read (that hits that code which seems to be more often than not) then
a few bytes are leaked. The dwPart is never being deleted.
I've fixed it with the attached patch. Leaking memory everytime a
message is read is bad news so I'll commit if no one objects.
BTW: Sorry about the whitespace changes in the patch.
Don.
["memleak" (text/x-diff)]
--- kmail-commands/kmreaderwin.cpp Mon Sep 2 16:39:18 2002
+++ kmail/kmreaderwin.cpp Mon Sep 2 21:58:25 2002
@@ -1865,7 +1865,6 @@ void KMReaderWin::initHtmlWidget(void)
// Espen 2000-05-14: Getting rid of thick ugly frames
mViewer->view()->setLineWidth(0);
-
connect(mViewer->browserExtension(),
SIGNAL(openURLRequest(const KURL &, const KParts::URLArgs &)),this,
SLOT(slotUrlOpen(const KURL &, const KParts::URLArgs &)));
@@ -2895,7 +2894,7 @@ void KMReaderWin::parseMsg(KMMessage* aM
else
s += "false";
s += "\n#######\n#######";
-kdDebug(5006) << s << endl;
+ kdDebug(5006) << s << endl;
mColorBar->setEraseColor( QColor( "white" ) );
mColorBar->setText("");
@@ -2913,7 +2912,6 @@ kdDebug(5006) << s << endl;
type = aMsg->typeStr();
numParts = aMsg->numBodyParts();
-
int mainType = aMsg->type();
int mainSubType = aMsg->subtype();
QString mainCntTypeStr;
@@ -2951,16 +2949,12 @@ kdDebug(5006) << s << endl;
mainBody->Parse();
}
- if( mRootNode ) {
if( onlyProcessHeaders )
savedRootNode = mRootNode;
else
delete mRootNode;
- }
- mRootNode = new partNode( mainBody ? mainBody : 0,
- mainType,
- mainSubType );
+ mRootNode = new partNode( mainBody, mainType, mainSubType, true );
mRootNode->setFromAddress( aMsg->from() );
QString cntDesc, cntSize, cntEnc;
@@ -3015,7 +3009,6 @@ kdDebug(5006) << "\n ------ Sorry,
writePartIcon(&vCardNode->msgPart(), aMsg->partNumber(vCardNode->dwPart()), TRUE );
}
}
-
queueHtml("<div id=\"header\">"
+ (writeMsgHeader(aMsg, hasVCard))
+ "</div><div><br></div>");
@@ -3038,7 +3031,6 @@ kdDebug(5006) << "\n ------ Sorry,
aMsg->setSignatureState( signatureState );
bool emitReplaceMsgByUnencryptedVersion = false;
-
// note: The following define is specified on top of this file. To compile
// a less strict version of KMail just comment it out there above.
#ifdef STRICT_RULES_OF_GERMAN_GOVERNMENT_02
@@ -3115,14 +3107,11 @@ kdDebug(5006) << "KMReaderWin - attach
// save current main Content-Type before deleting mRootNode
int rootNodeCntType = mRootNode ? mRootNode->type() : DwMime::kTypeUnknown;
-
// if necessary restore original mRootNode
- if( savedRootNode ) {
- if( mRootNode )
+ if(onlyProcessHeaders) {
delete mRootNode;
mRootNode = savedRootNode;
}
-
// store message id to avoid endless recursions
setIdOfLastViewedMessage( aMsg->msgId() );
@@ -4326,7 +4315,7 @@ void KMReaderWin::resizeEvent(QResizeEve
//-----------------------------------------------------------------------------
void KMReaderWin::slotDelayedResize()
{
- //mViewer->widget()->setGeometry(0, 0, width(), height());
+// mViewer->widget()->setGeometry(0, 0, width(), height());
mBox->setGeometry(0, 0, width(), height());
}
@@ -4881,7 +4870,6 @@ void KMReaderWin::slotDocumentChanged()
//-----------------------------------------------------------------------------
void KMReaderWin::slotTextSelected(bool)
{
-
QString temp = mViewer->selectedText();
kapp->clipboard()->setText(temp);
}
--- kmail-commands/partNode.h Mon Aug 12 19:50:58 2002
+++ kmail/partNode.h Mon Sep 2 20:51:48 2002
@@ -73,7 +73,8 @@ private:
public:
partNode( DwBodyPart* dwPart,
int explicitType = DwMime::kTypeUnknown,
- int explicitSubType = DwMime::kSubtypeUnknown ) :
+ int explicitSubType = DwMime::kSubtypeUnknown,
+ bool deleteDwBodyPart = false ) :
mRoot( 0 ),
mPrev( 0 ),
mNext( 0 ),
@@ -85,7 +86,7 @@ public:
mIsSigned( false ),
mMsgPartOk( false ),
mEncodedOk( false ),
- mDeleteDwBodyPart( false ),
+ mDeleteDwBodyPart( deleteDwBodyPart ),
mMimePartTreeItem( 0 ) {
if( explicitType != DwMime::kTypeUnknown ) {
mType = explicitType; // this happens e.g. for the Root Node
_______________________________________________
KMail Developers mailing list
kmail@mail.kde.org
http://mail.kde.org/mailman/listinfo/kmail
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic