Closed Bug 1394524 Opened 3 years ago Closed 2 years ago

Mail with ICS attachment which contains invalid characters and causes "[calIICSService.parseICS] calIcsParser.js:145" stays always "Unread" unless marked "as read" manually

Categories

(Calendar :: E-mail based Scheduling (iTIP/iMIP), defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: martin.greil, Assigned: MakeMyDay)

Details

Attachments

(4 files, 2 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/59.0.3100.0 Iron Safari/537.36

Steps to reproduce:

Open the attached Mail and send it to yourself.


Actual results:

The mail list entry stays bold after read.


Expected results:

After reading, the mail should change status from "unread" to "read".
Attachment #8901919 - Attachment mime type: message/rfc822 → text/plain
I edited the message "as new" and sent it to myself. Upon receipt, it looks unread and opening it or displaying it in the preview pane, doesn't make it read. However, marking it as read works, but when marking it at unread, the same original effect occurs. In fact, you don't need to go though sending the message, just import (drag into a folder or the Trash) and mark it Unread.

I've changed the MIME header of the text/calendar attachment to text/plain and the problem goes away.

The problem is that Calendar/Lightning runs into an error which is logged on the console:
Component returned failure code: 0x804a0100 [calIICSService.parseICS] calIcsParser.js:145

So basically the ICS file is invalid and the system chokes on it and doesn't mark the message read. The decoded content of the ICS file is:

BEGIN:VCALENDAR
VERSION:2.0
PRODID:-//ince/events//NONSGML v1.0//EN
BEGIN:VEVENT
DTSTART:20170828T060000Z
DTEND:20170828T061500Z
DESCRIPTION:Tesla Service Appointment Confirmation
SUMMARY:Tesla Service Appointment Confirmation
LOCATION:Nuremberg-Thumenberger
DTSTAMP:20170808T142343Z
SEQUENCE:1
UID:1592763
END:VEVENT
BEGIN:VALARM
TRIGGER:-PT30M
REPEAT:2
DURATION:PT15M
ACTION:DISPLAY
END:VALARM
END:VCALENDAR


So there is an encoding problem and there are invalid characters in the file. That throws the ICS decoder which doesn't appear to handle the case gracefully and the message doesn't get marked as read when reading it.

It's a bug but not the most severe one we have on file ;-)
Status: UNCONFIRMED → NEW
Component: Folder and Message Lists → E-mail based Scheduling (iTIP/iMIP)
Ever confirmed: true
Product: Thunderbird → Calendar
Summary: Mail with Attachment.ics stays always "Unread" → Mail ICS attachment which contains invalid characters and causes "[calIICSService.parseICS] calIcsParser.js:145" stays always "Unread" unless marked "as read" manually
Version: 52 Branch → unspecified
Summary: Mail ICS attachment which contains invalid characters and causes "[calIICSService.parseICS] calIcsParser.js:145" stays always "Unread" unless marked "as read" manually → Mail with ICS attachment which contains invalid characters and causes "[calIICSService.parseICS] calIcsParser.js:145" stays always "Unread" unless marked "as read" manually
Attachment #8902205 - Attachment mime type: message/rfc822 → text/plain
In what version of Lightning do you use? Do you calendar.icaljs enabled in the advanced preferences? Does this issue also occur for you, if you flip this pref (requires restart of TB afterwards)?
Flags: needinfo?(martin.greil)
I was using Thunderbird 52.3.0 (32-Bit)

Today there was an update to 52.4.0
There is Lightning 5.4.4 included.
The reported bug is still here.

My setting is standard:
Name            / Status   / Type    / Value
calendar.icaljs / Standard / boolean / false

After set to true and restart of TB the reported bug is still here.
Now I'm switching back to false.
Flags: needinfo?(martin.greil)
This patch adds the capability to catch errors in the ics parser to deal with the issue in the first place.

Additionally, ical.js and libical are dealing differently with invalid ics. Libical is more gracefully atm and wrapps invalid properties/params in an X-LIC-ERROR property/param. I added some code to get icaljs to do the same - before I go further and make this ready for an upstream pr including tests, I would like to get some feedback on whether this is the road you would like to take for that. Or would we need a config option to make this behaviour optionally to not break other users of ical.js relying on the current behaviour?
Assignee: nobody → makemyday
Status: NEW → ASSIGNED
Attachment #8921601 - Flags: feedback?(philipp)
Comment on attachment 8921601 [details] [diff] [review]
FailGracefullyOnIcaljsParsing-V1.diff

Honestly, I think X-LIC-ERROR should DIAF. I'm not sure what the best alternative is, either just ignoring the line completely or erroring out and catching the error somewhere. If it is on the parameter level then of course the same for just the parameter.
Attachment #8921601 - Flags: feedback?(philipp) → feedback-
Fien for me. However, we should implement a more gracefully error handling in icaljs compared to its current behaviour of throwing - from a user's perspective, this looks like a shortcoming of icaljs, because libical is able to parsse also invalid ics so these can be displayed.

But I deferr this to bug 1411530 and go here with just catching and logging errors in the ics parser. As long as X-LIC exists in our code base, I still would like to make use of it for logging purposses here.
Attachment #8921601 - Attachment is obsolete: true
Attachment #8921816 - Flags: review?(philipp)
Comment on attachment 8921816 [details] [diff] [review]
FailGracefullyOnIcsParsing-V1.diff

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

The ical.js changes need to go upstream before pushing here. Please send a PR for this change there.
Attachment #8921816 - Flags: review?(philipp) → review+
Huh, the patch was only cleaned up half way before upload. Obviously, I didn't remove the ical.js changes, which was intended due to comment 6.

This version just removes these useless changes and retains the code catching the exceptions, so I'm carrying forward the r+.

Philipp, if you have changed your mind regarding adding X-LIC props to ical.js, please leave a comment or file a follow-up bug to do so.
Attachment #8921816 - Attachment is obsolete: true
Attachment #8937234 - Flags: review+
Keywords: checkin-needed
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/8c2e2bb95746
Fail gracefully when parsing invalid ics data. r=philipp
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 6.1
Now I'm using Thunderbird 52.9.1 (32-Bit).
I see the issue again: Got an email with attached "Appointment.ics" and after selecting that mail it stays bold.

After reading, the mail should change status from "unread" to "read".
If needed, I can upload/forward that mail with attachment to someone.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Example to explain bug in Thunderbird 52.9.1 (32-Bit)
Attachment #8997698 - Flags: feedback-
This bug is fixed in Lightning 6.1 and newer. You are using Lightning 5.4 and therefore you will still see the problem until new major release is done or unless you switch to Beta channel.
Thank You very much.
There is no point to reopen this if you see the issue in a not fixed version, so setting this back to fixed. ESR650 is up to be releast soon. If you're experiencing the issue in that version, too, feel free to file a new bug.
Status: REOPENED → RESOLVED
Closed: 3 years ago2 years ago
Resolution: --- → FIXED
That's TB 60 ESR. You can get a preview here:
https://archive.mozilla.org/pub/thunderbird/candidates/60.0-candidates/build4/
Remove your installed Calendar, reset pref extensions.installedDistroAddon.{e2fda1a4-762b-4020-b5ad-a41df1933103} and restart TB.
You need to log in before you can comment on or make changes to this bug.