189988 - div moz-text-flowed added to plain text message when forwarded as attachment

Status: VERIFIED FIXED

(SeaMonkey :: MailNews: Message Display, defect)

Description

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

moz-text-flowed in message, caused by forwarding plain text message.

more details and screen shots coming soon.

Comment 1

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

stack trace of when we add the 

<div class="moz-text-flowed" style="font-family: -moz-fixed">

MimeInlineTextPlainFlowed_parse_begin(MimeObject * 0x06e0f8b0) line 200
MimeMessage_close_headers(MimeObject * 0x06ddb9a8) line 484 + 12 bytes
MimeMessage_parse_line(char * 0x06e0e480, int 2, MimeObject * 0x06ddb9a8) line 
282 + 9 bytes
convert_and_send_buffer(char * 0x06e0e480, int 2, int 1, int (char *, unsigned 
int, void *)* 0x03738cd0 MimeMessage_parse_line(char *, int, MimeObject *), 
void * 0x06ddb9a8) line 168 + 15 bytes
mime_LineBuffer(const char * 0x06e0e415, int 44, char * * 0x06ddb9d0, int * 
0x06ddb9d8, unsigned int * 0x06ddb9e0, int 1, int (char *, unsigned int, void *)
* 0x03738cd0 MimeMessage_parse_line(char *, int, MimeObject *), void * 
0x06ddb9a8) line 255 + 29 bytes
MimeObject_parse_buffer(char * 0x06e0e288, int 441, MimeObject * 0x06ddb9a8) 
line 260 + 49 bytes
mime_display_stream_write(_nsMIMESession * 0x06ddbaa8, const char * 0x06e0e288, 
int 441) line 898 + 20 bytes
nsStreamConverter::OnDataAvailable(nsStreamConverter * const 0x06dd88b8, 
nsIRequest * 0x06ddc060, nsISupports * 0x06ddbcf4, nsIInputStream * 0x06dde94c, 
unsigned int 0, unsigned int 441) line 949 + 24 bytes
nsImapCacheStreamListener::OnDataAvailable(nsImapCacheStreamListener * const 
0x06dd6ea8, nsIRequest * 0x06dde840, nsISupports * 0x06ddbcf4, nsIInputStream * 
0x06dde94c, unsigned int 0, unsigned int 441) line 7459 + 51 bytes
nsInputStreamPump::OnStateTransfer() line 386 + 65 bytes
nsInputStreamPump::OnInputStreamReady(nsInputStreamPump * const 0x06dde844, 
nsIAsyncInputStream * 0x06dde94c) line 316 + 11 bytes
nsInputStreamReadyEvent::EventHandler(PLEvent * 0x05530a14) line 102
PL_HandleEvent(PLEvent * 0x05530a14) line 663 + 10 bytes
PL_ProcessPendingEvents(PLEventQueue * 0x00ef7720) line 593 + 9 bytes
_md_EventReceiverProc(HWND__ * 0x0017011a, unsigned int 49503, unsigned int 0, 
long 15693600) line 1385 + 9 bytes
USER32! 77e3a290()
USER32! 77e145b1()
USER32! 77e1a752()
nsAppShellService::Run(nsAppShellService * const 0x015b5db0) line 471
main1(int 2, char * * 0x00271d80, nsISupports * 0x00f94fb0) line 1556 + 32 bytes
main(int 2, char * * 0x00271d80) line 1928 + 37 bytes
mainCRTStartup() line 338 + 17 bytes
KERNEL32! 77e9ca90()

Comment 2

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

ducarroz thinks this might have been caused by the fix for #161488

I'll double check to see if he's right, and then elaborate on a possible fix 
that ducarroz suggested.

Comment 3

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

Attachment: patch based on a suggestion from ducarroz

Comment 4

[Jean-Francois Ducarroz]

This patch will fix that specific case but not all of them like plain text not
flowed! We should be able to do better by preventing the mine decoder to use a
specific clazz once the smime layer has been decoded.

My suggestion is that the function mime_find_class (mimeei.cpp), unless the
found class is mimeEncryptedCMSClass, should always return
mimeExternalObjectClass when obj->options->format_out ==
nsMimeOutput::nsMimeMessageDecrypt

Comment 5

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

Comment on attachment 112199 [details] [diff] [review]
patch based on a suggestion from ducarroz

reject by ducarroz.

note, I've verified that #161488 did cause this.

Comment 6

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

Attachment: ducarroz's idea, but I'm not sure this is where he'd put it.

Comment 7

[Jean-Francois Ducarroz]

Comment on attachment 112207 [details] [diff] [review]
ducarroz's idea, but I'm not sure this is where he'd put it.

R=ducarroz

Comment 8

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

fixed

Comment 9

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

actually, I checked in this (after the tree broke)

#ifdef ENABLE_SMIME
  // see bug #189988
  if (opts && opts->format_out == nsMimeOutput::nsMimeMessageDecrypt &&
       (clazz != (MimeObjectClass *)&mimeEncryptedCMSClass &&
        clazz != (MimeObjectClass *)&mimeMultipartSignedCMSClass)) {
    clazz = (MimeObjectClass *)&mimeExternalObjectClass;
  }
#endif

I'll double check with ducarroz about the mimeMultipartSignedCMSClass part.

Comment 10

[Jean-Francois Ducarroz]

I am not convinced that we need to exclude mimeMultipartSignedCMSClass as well!
The only thing we need to decode is the encryption layer around a message. The
fact the message is signed should not request extra treatment. Kaie, am I right?

Comment 11

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

ah, are you saying that we only "de-crypt" encrypted messages, we don't "de-
sign" signed messages (when forwarding?)

Comment 12

[Jean-Francois Ducarroz]

correct. The goal of nsMimeOutput::nsMimeMessageDecrypt is to make sure the
recipient will be able to read the message. The fact the message is signed or
not does not affect the reading. Also, we want to preserve as much as possible
the original message. 

Comment 13

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

ok, thanks ducarroz.  I'll fix it to be this when the tree opens


#ifdef ENABLE_SMIME
  // see bug #189988
  if (opts && opts->format_out == nsMimeOutput::nsMimeMessageDecrypt &&
       (clazz != (MimeObjectClass *)&mimeEncryptedCMSClass)) {
    clazz = (MimeObjectClass *)&mimeExternalObjectClass;
  }
#endif

Comment 14

[Kai Engert [:KaiE:]]

I think we should do some additional tests, because there exists a mixed condition.

There are two different approaches to produce a signed S/Mime message. Both
approaches are allowed per S/Mime RFCs, and both are effectively used.

While we produce only one of those, other mail clients produce both, and for
compatibility reasons, we have support for both.

The problem is that some signed messages look very much like an encrypted
message. They use a similar encoding approach, and it appears the message is
encrypted (because you only see encoded data when using view source), but in
fact the message is not encrypted, only encoded.

Because this variation of signed messages look so similar to encrypted messages,
the portion of our S/Mime engine that cares for encrypted messages, actually
cares for those signed only messages, too.

That is, if a message is handleded by the mimeEncryptedCMS class, it is not
necessarily an encrypted message, it might be signed only.

I'm not sure whether the condition you are checking is correct for such messages.


I would like to suggest that we do some additional testing before landing this fix.

Can you describe how I could see the problem you are trying to fix with this
bug? I would like to test, whether all kinds of encrypted/signed messages are
sill being handled correctly.

Thanks!

Comment 15

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

to test the bug I'm trying to fix:

forward a plain text email as an attachment.

The body of the forwarded message is wrapped with this:

<div class="moz-text-flowed" style="font-family: -moz-fixed">
</div>

I'll screen shot for you.

Comment 16

[Kai Engert [:KaiE:]]

Thanks, I'm able to reproduce the problem now. Doing some tests and will give
feedback.

Comment 17

[Kai Engert [:KaiE:]]

I now understand better what this patch is doing. Because you check for
nsMimeOutput::nsMimeMessageDecrypt, this new code only affects the recently
introduced mime decoding step, that is done in order to decrypt any encrypted
content.

The issue I mentioned doesn't require separate handling. In both cases, whether
it's an opaquely signed message or an encrypted message, mimeEncryptedCMSClass
will be used, and in both cases the handling is required.

Because your patch only changes the handling for other clazzes, and because no
decoding is required for multipart signed messages, I assume your handling of
those parts as mimeExternalObjectClass makes sense.


So to summarize:

  encrypted messages use mimeEncryptedCMSClass,
    decoding is necessary

  opaque signed messages use mimeEncryptedCMSClass,
    decoding is currently done,
    altough it is not absolutely necessary.
    (We lose the signature information for opaque signed messages,
     maybe we could improve this handling in the future,
     but that is a separate problem.)
    I believe your patch is not changing our current behaviour
    and makes therefore sense.

  multipart signed messages use mimeMultipartSignedCMSClass,
    no decoding is necessary


So, to put it into different words, we DO de-sign some signed only messages,
namely those signed messages that are opaque signed messages
handled by mimeEncryptedCMSClass,

but we don't de-sign multipart signed messages,
and therefore the latest patch is correct, which does not try
to decode mimeMultipartSignedCMSClass.


(I'll file a separate bug that suggests we could improve the decoding phase in
the future, by only trying to decrypt, if the handler clazz is
mimeEncryptedCMSClass, and the content is indeed an encrypted message, not
simply an opaque signed message.)

Comment 18

[Kai Engert [:KaiE:]]

Comment on attachment 112207 [details] [diff] [review]
ducarroz's idea, but I'm not sure this is where he'd put it.

r=kaie, thanks a lot for the fix!

Comment 19

[Kai Engert [:KaiE:]]

I would like to reopen the bug. While the special cases make sense to me, the
patch seems to make our behaviour worse.

When forwarding a plain text message as attachment, the attachment in the
received message is shown as a message with full headers. That might be acceptable.

However, when forwarding an encrypted message with a spreadsheet attachment,
encrypting the message again, the received message is displayed in raw form,
with the attached base64 data shown as plain text. This works better in Mozilla
1.2.1, where the received message is correctly displayed as a nicely formatted
message and the included binary attachment listed in the box of attachments.

I will make an additional test to check whether the different behaviour is
indeed caused by this single patch.

Comment 20

[Kai Engert [:KaiE:]]

Comment on attachment 112207 [details] [diff] [review]
ducarroz's idea, but I'm not sure this is where he'd put it.

To avoid confusion, marking this bug obsolete, since it doesn't have the
ENABLE_SMIME protection.

Comment 21

[Kai Engert [:KaiE:]]

I think the situation is more complicated, but in short, seems to be unrelated
to this bug.


The new broken behaviour that I described in my previous comment is unrelated to
the patch posted in this bug.

The broken behaviour can be seen when forwarding a signed/encrypted message,
containing a binary attachment, using the latest trunk, regardless whether the
patch from this bug is used or not.


Seth, if you want to go ahead and correct the checkin by changing it to what you
describe in comment 13, I think that's fine.


I will file a separate bug for the other regression I see. For completeness, I
have archived a special build, which is identical to 1.2.1, but has the patches
from bug 161488, bug 179082 and bug 55657. This special build does not show the
regression I'm mentioning. Something must have happened separate to those bugs
after 1.2.x.

Comment 22

[Kai Engert [:KaiE:]]

(Update on the separate "regression" I reported. This was only in my own build.
I couldn't reproduce the problem with any of the recent nightly builds from
mozilla.org. What I saw was a side effect from the mistake made when fixing bug
189591. So, all is fine, no regression.)

Comment 23

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

Attachment: patch, heeding ducarroz's suggestion

Comment 24

[Christopher Blizzard (:blizzard)]

Comment on attachment 112493 [details] [diff] [review]
patch, heeding ducarroz's suggestion

a=blizzard for 1.3b

Comment 25

[Kai Engert [:KaiE:]]

Comment on attachment 112493 [details] [diff] [review]
patch, heeding ducarroz's suggestion

r=kaie

Comment 26

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

fixed.

for the record, here's what's in the tree:

#ifdef ENABLE_SMIME
  // see bug #189988
  if (opts && opts->format_out == nsMimeOutput::nsMimeMessageDecrypt &&
       (clazz != (MimeObjectClass *)&mimeEncryptedCMSClass)) {
    clazz = (MimeObjectClass *)&mimeExternalObjectClass;
  }
#endif

thanks again to ducarroz and kaie on this one.

Comment 27

[esther]

using trunk build 20030218 on winxp, maco osx and linux this is fixed.  The
attached plain text message no longer diplays this: <div class="moz-text-flowed"
style="font-family: -moz-fixed"> </div> around the message body in the message
pane window.