[Kde-pim] Hunting down #291171 and other filtering issues

[Andras Mantia]

Hi,

Szymon Tomasz Stefanek wrote:

> Hi all!
> 
> Tonight #291171 made me mad enough: I can no longer stand it.
> I either fix it or throw the computer out of the window :D

Thanks for looking at it. :)
> 
> The bug is related to filters that break the Content-Type header turning
> multipart/mixed data into text/plain.
> 
> [ see https://bugs.kde.org/show_bug.cgi?id=291171 ]
> 
> I've hunted it and I need a hint.
> 
> In turns out that it's an issue related to how KMime::Message manages
> multipart data. The KMime::Content::parse() method decodes the body
> of the message from its "body" member. If the message is a multipart one
> then the "body" member will be cleared as the data is stored in a separate
> list of contents (and the documentation even states that).
> 
> Now the filter components (search patterns and actions) tend to call the
> parse() method multiple times. 

That is wrong and it is documented in KMime::Content::parse():
"
Parses the Content.

This means the broken-down object representation of the Content is updated 
from the string representation of the Content.

Call this if you want to access or change headers, sub-Contents or the 
encapsulated message.

Note:
Calling parse() twice will not work for multipart contents or for contents 
of which the body is an encapsulated message. The reason is that the first 
parse() will delete the body, so there is no body to work on for the second 
call of parse().
Calling this will reset the message returned by bodyAsMessage(), as the 
message is re-parsed as well. Also, all old sub-contents will be deleted, so 
any old Content pointer will become invalid.
Reimplemented in KMime::Message, and KMime::NewsArticle. "

I know, this is bad, but that's the API and we can't change in KDE 4.x the 
behavior.

> Now there may be different approaches to a fix for this.
> 
> 1) Fix the filtering bug only: remove all the parse() calls from the
> filters as Akonadi seems to call parse() itself at the moment the message
> is fetched. KMime never tries to parse an empty body and it never updates
> the Content- Type header... maybe.

Due to above this sounds like a sane approach.

> 2) Change KMime::Content::parse() to NOT wipe out the body member.
> This will allow the method to be called multiple times. However there
> might be some code around that relies on the empty body after parse() ?

Exactly, this would be behavior-incompatible change. IIRC I already did this 
once ~2 years ago, and the change was rejected. But maybe it was about 
assemble() and not parse() :)

> 2.a) Change KMime::Content::parse() to store the message body in an
> auxiliary var and let the body member be cleared. When parse() is called
> again with the body member empty try to restore it from auxiliary.
> Possibly backward compatible but will make the multipart messages twice as
> big in RAM. I've actually tried this: it fixes the bug.

Hm. KMime already uses a lot of memory, so I'm not sure if this is very 
good.

> 3) Fix KMime::Content::parse() by making it a null operation if it has
> already been called and the contents weren't reset. Looks like having less
> side effects but I'm not really sure.

Yes, same problem as in 2).

> Which way would you suggest? 1? 2? 3? Something else?

I'd prefer 1 even if that is the most hacky solution. 

Andras


_______________________________________________
KDE PIM mailing list kde-pim at kde.org
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/