1372928 - In "message body as plaintext" mode, invitation are not detected as so and do not have .ics file attached

Status: RESOLVED FIXED

(MailNews Core :: MIME, defect)

Description

[L.I.A.R.]

Attachment: b1+2.png

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:53.0) Gecko/20100101 Firefox/53.0
Build ID: 20170524174214

Steps to reproduce:

Preliminary note: since I don't want anybody to execute unknown code on my machine (who wants that????), my ThB is configured to "View" "Message Bod[ies]" "As Plain Text"

I received an email that is an invitation to a meeting.


Actual results:

1 - email is not detected as an invitation and Thb+lightning does not propose to Accept/refuse
2 - the invitation is not shown (not even as an attached .ics)


Expected results:

- Invitation text should appear in the body
- An ics file should appear as attachments
- Lightning detect that it's an invitation and propose to accept/reject it

Might be related to bug XXX

Comment 1

[L.I.A.R.]

bug XXX == https://bugzilla.mozilla.org/show_bug.cgi?id=1372935

PS: the sender was an Apple user. That's what makes me think the problem lies in the ("Plain Text" View mode) vs. ("Orig HTML" View mode)

Comment 2

[Jorg K (CEST = GMT+2)]

Can you attach such a message for inspection here: Save/drag to the desktop, edit resulting .eml file with editor to remove personal information, then attach here, "Attach File" above.

Comment 4

[[:MakeMyDay]]

(In reply to Jorg K (GMT+2) from comment #2)
> Can you attach such a message for inspection here: Save/drag to the desktop,
> edit resulting .eml file with editor to remove personal information, then
> attach here, "Attach File" above.

There are reports on irc that this has been working in 45.8. I have done some tests with TB52.4: for messages with a single text/calendar part this seems to work also in plain text mode, while for multipart messages the calendar part is not detected. In any of the HTML modes, it works like expected.

To reproduce in Daily yourself you can take any mulktipart message with an text/calendar part, regardless whether this is multipart/alternative or multipart/mixed. Just switch in TB menu View->Message Body As to Plain Text. This is also an issue if you codewise effectively disable the display changes feature, so it's not related to bug 1360155, but seems to be a mime issue / regression.

Comment 5

[Jorg K (CEST = GMT+2)]

(In reply to [:MakeMyDay] from comment #4)
> To reproduce in Daily yourself you can take any mulktipart message with an
> text/calendar part, regardless whether this is multipart/alternative or
> multipart/mixed.
OK, I used attachment 8901591 [details] from bug 1360155 and it works just fine when viewing as plaintext in a current Daily.

So kindly attach an example that doesn't work.

Comment 6

[[:MakeMyDay]]

Attachment: invitation_example_not_displayed_in_plain_text_mode.eml

Sure. After some further testing, it seems to be limited to a multipart/alternative within a multipart/mixed with an additional ics attachment.

Comment 7

[Jorg K (CEST = GMT+2)]

Hmm, that message really has a quite unfortunate MIME structure:

multipart/mixed
  multipart/alternative
    text/plain
    text/calendar
attachment.

Since bug 253830 we prioritise the text part higher than anything else when in plaintext mode:
https://dxr.mozilla.org/comm-central/rev/952762964f408e33d060dd1110cac0626ae5fa51/mailnews/mime/src/mimemalt.cpp#432
text/plain gets PRIORITY_HIGHEST and text/calendar gets PRIORITY_NORMAL.

Before that bug which landed on TB 49, so not in TB 45 but in TB 52, all text parts had the same priority, so the last one won, maybe good for calendar, but not good if the last part was HTML that then got converted to plaintext, ignoring the dedicated plaintext part.

The simple fix is to move text/calendar up in priority if that's what's desired.

Comment 8

[Jorg K (CEST = GMT+2)]

Attachment: 1372928-plaintext-invite.patch (v1)

I guess you've heard about the Mailnews misery, so kindly try this for me and approve.

Comment 9

[Jorg K (CEST = GMT+2)]

Yes, I'll fix the trailing space.

Comment 10

[Jorg K (CEST = GMT+2)]

Attachment: 1372928-plaintext-invite.patch (v2)

(v1) wasn't right. You need more context than the patch contains to see that in plaintext mode both text/plain and text/calendar receive the highest priority.

Comment 11

[Jorg K (CEST = GMT+2)]

Comment on attachment 8924373 [details] [diff] [review]
1372928-plaintext-invite.patch (v2)

Or maybe Aceman also wants to take a look.

Comment 12

[:aceman]

Why do we need to play with priorities here? This shouldn't be a case of choosing one of many different variants of the text.
If there is a calendar, shouldn't it be shown in any case (regardless of plain/html display)? Can there be multiple calendars in a message? Shouldn't the calendar handled/shown in addition a the chosen body part, regardless of priorities?

Comment 13

[Jorg K (CEST = GMT+2)]

The decision of which part to display is done via priorities. The part with the highest priority wins, or the last part if two parts have the same priority. We won't change this design here.

In plaintext view mode, the plaintext part receives the highest priority to ensure it is displayed and not the HTML part (downgraded to plaintext) which typically comes later.

What the author of bug 253830, Terje, and the reviewer, myself, overlooked is that when text/calendar follows text/plain in plaintext mode, text/calendar should receive the same (highest) priority that text/plain receives. So if Lightning is installed, the last part with that high priority, so the text/calendar part is displayed, or better, passed to Lightning via some complicated process to display in its own special way.

The patch corrects that oversight with one simple if-clause. That the 100% correct fix.

Comment 14

[Jorg K (CEST = GMT+2)]

Actually: That *is* the 100% correct fix.

Comment 15

[[:MakeMyDay]]

Comment on attachment 8924373 [details] [diff] [review]
1372928-plaintext-invite.patch (v2)

Review of attachment 8924373 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, it works like expected with this patch. Too me, this approach seems reasonable, so r+ from my side (Although I'm not a peer here, so please decide yourself whether this is sufficient for landing).

If there is more then one event in the email or other parts should be displayed additionally, the email should have been composed not using multipart/alternate alone but just multipart/mixed (for which displaying the events already works). For providing an option to switch between a text/calendar or text/plain resp. text/html for multipart/alternate messages, there is with bug 1406868 an enhancement request on file, so we should keep things separate here.

Comment 16

[:aceman]

(In reply to Jorg K (GMT+2) from comment #13)
> So if Lightning is installed, the last part with that high
> priority, so the text/calendar part is displayed, or better, passed to
> Lightning via some complicated process to display in its own special way.

I just do not understand why the calendar part has to compete with the plain/html parts of messages to get displayed. I think priority should be computed for the message parts, but calendar part should be sent to Lightning in any case and that decides how to display it.

Comment 17

[Jorg K (CEST = GMT+2)]

Feel free to rework it and deal with the fallout :-(

This is the second regression from bug 253830 and its friend, bug 574989, there was also bug 1334937.

Currently all parts compete, that's the architecture, and if the winning part is text/calendar, we ship it off to Lightning to determine its display.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/5014946f75dc
prioritise text/calendar the same as text/plain in plaintext mode so inviation is displayed. r=MakeMyDay

Comment 19

[Jorg K (CEST = GMT+2)]

Beta (TB 57):
https://hg.mozilla.org/releases/comm-beta/rev/83fd8361714142ed4179db701cbfcd0389c52b95

Comment 20

[:aceman]

In the push with this patch the test run fine on c-c. But in all later runs the test fails again on linux 32bit.
See e.g. https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=75d12f0aeb238a399919f1e3a911597b850934bd

Comment 21

[Jorg K (CEST = GMT+2)]

I don't understand anything here. Which test? Which patch? Did you post this to the wrong bug?
And that push shows bustage from bug 1414521 and bug 1414533, so IMHO it's not really useful as a comparison.

Comment 22

[:aceman]

Yeah sorry, this was to be posted in bug 1351753.

Comment 23

[Jorg K (CEST = GMT+2)]

Tb 52.5 ESR (should be tracking 57+):
https://hg.mozilla.org/releases/comm-esr52/rev/655fb6d54a07