VALARM Attachment is read in twice

RESOLVED FIXED in 6.7

Status

RESOLVED FIXED
9 months ago
5 days ago

People

(Reporter: code, Assigned: Fallen)

Tracking

Lightning 5.4.7

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

9 months ago
Created attachment 8974103 [details]
foobar_NC.ics

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/66.0.3359.139 Safari/537.36

Steps to reproduce:

Create a calendar on a webdav server, e.g. nextcloud.

Alternatively do:
a) Import attached ics file to the calendar using the nextcloud web interface. (Thunderbird will not import the ics because it thinks the VALARM is invalid (?)).
b) The 'real' thing that creates this scenario: Create a recurring event and let Apple iCalendar modify it to add a VALARM reminder. Modify a single event out of the recurring series, so that there are multiple events with the same UID.

Subscribe to the CalDAV calendar in Thunderbird.

Try to modify a single event in the series. E.g. the Friday occurence to be named FOO Fri.


Actual results:

Event is not updated. Server responds with:
Fatal	webdav	Sabre\DAV\Exception\UnsupportedMediaType: Validation error in iCalendar: ATTACH MUST NOT appear more than once in a VALARM component

Thunderbird Error Log:
19:15:20.354 CalDAV: Unexpected status modifying item to testcalendar: 415
BEGIN:VCALENDAR
PRODID:-//Mozilla.org/NONSGML Mozilla Calendar V1.1//EN
VERSION:2.0
BEGIN:VTIMEZONE
TZID:Europe/Berlin
BEGIN:DAYLIGHT
TZOFFSETFROM:+0100
TZOFFSETTO:+0200
TZNAME:CEST
DTSTART:19700329T020000
RRULE:FREQ=YEARLY;BYDAY=-1SU;BYMONTH=3
END:DAYLIGHT
BEGIN:STANDARD
TZOFFSETFROM:+0200
TZOFFSETTO:+0100
TZNAME:CET
DTSTART:19701025T030000
RRULE:FREQ=YEARLY;BYDAY=-1SU;BYMONTH=10
END:STANDARD
END:VTIMEZONE
BEGIN:VEVENT
CREATED:20180508T165324Z
LAST-MODIFIED:20180508T171519Z
DTSTAMP:20180508T171519Z
UID:5fd8c981-8cf6-4522-a336-7a0817317f23
SUMMARY:FOO
RRULE:FREQ=DAILY;UNTIL=20180513T101500Z
EXDATE:20180509T101500Z
DTSTART;TZID=Europe/Berlin:20180507T121500
DTEND;TZID=Europe/Berlin:20180507T131500
TRANSP:OPAQUE
X-MOZ-GENERATION:3
SEQUENCE:1
END:VEVENT
BEGIN:VEVENT
CREATED:20180508T165341Z
LAST-MODIFIED:20180508T165351Z
DTSTAMP:20180508T165351Z
UID:5fd8c981-8cf6-4522-a336-7a0817317f23
SUMMARY:FOO BAR
RECURRENCE-ID;TZID=Europe/Berlin:20180510T121500
DTSTART;TZID=Europe/Berlin:20180510T121500
DTEND;TZID=Europe/Berlin:20180510T131500
TRANSP:OPAQUE
X-MOZ-GENERATION:1
BEGIN:VALARM
ACTION:AUDIO
TRIGGER;VALUE=DURATION:-PT5M
ATTACH:Basso
X-WR-ALARMUID:28F8007B-FE56-442E-917C-1F4E48DD406A
UID:28F8007B-FE56-442E-917C-1F4E48DD406A
X-APPLE-DEFAULT-ALARM:TRUE
ATTACH:Basso
END:VALARM
END:VEVENT
BEGIN:VEVENT
CREATED:20180508T171508Z
LAST-MODIFIED:20180508T171519Z
DTSTAMP:20180508T171519Z
UID:5fd8c981-8cf6-4522-a336-7a0817317f23
SUMMARY:FOO Fri
RECURRENCE-ID;TZID=Europe/Berlin:20180511T121500
DTSTART;TZID=Europe/Berlin:20180511T121500
DTEND;TZID=Europe/Berlin:20180511T131500
TRANSP:OPAQUE
X-MOZ-GENERATION:3
SEQUENCE:1
END:VEVENT
END:VCALENDAR
 1 calDavCalendar.js:825:21

As it is evident from the error message, the modified event has a duplicate ATTACH:Basso in the VALARM. 



Expected results:

Thunderbird should correctly generate the new series VEVENTS without duplicate parameters
(Reporter)

Updated

8 months ago
Component: General → Provider: ICS/WebDAV
OS: Unspecified → All
Hardware: Unspecified → All
(Reporter)

Comment 1

8 months ago
I don't know if this is a bug in WebCal or ICS/WebDAV component, so I initially left the assignment as General. But since nobody seems to react I am trying ICS/WebDAV now.
(Reporter)

Comment 2

8 months ago
Okay, After digging into the Debugger I have found something. If I add ATTACH to promotedProps in calAlarm.js then the ATTACH Property of the VALARM is not parsed twice. At least that's what I think it does. Can a developer please confirm this is the right fix?

After applying the patch, the alarm gets parsed correctly. And above described editing of an unrelated recurrent exceptional event within the same series is successful.

Strangely enough when a single event is parsed, the ATTACH also appears twice in the resulting object, but when editing that event, the alarm is completely lost, so there is never that duplicate ATTACH in the VALARM which the server complains about. The same happens when the recurring exceptional event which has the alarm itself is edited (Provided no other recurring event in that series with its own VALARM throws the error).
Component: Provider: ICS/WebDAV → Alarms
(Reporter)

Comment 3

8 months ago
Created attachment 8976092 [details] [diff] [review]
tb-calendar-alarm.patch
(Reporter)

Comment 4

8 months ago
Comment on attachment 8976092 [details] [diff] [review]
tb-calendar-alarm.patch

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

I would really like to have this bug and patch reviewed by some developer. Our secretary is really stressed out that she cannot edit events for her boss.
Attachment #8976092 - Flags: review?(philipp)
Assignee: nobody → code
Comment on attachment 8976092 [details] [diff] [review]
tb-calendar-alarm.patch

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

Sorry for the delay. This isn't quite the right patch, as attachments are handled separately instead of as promoted props. The events generated there are going against spec, there must only be one ATTACH in an AUDIO alarm.

What I could imagine doing (which would fix this case) is to only throw an error at https://searchfox.org/comm-central/source/calendar/base/src/calAlarm.js#329 if the value (and parameters) of the newly added audio alarm is different than the value of the already existing alarm. If the value is the same, silently drop the newly added alarm.

Do you think you could look into creating a patch to that effect?
Attachment #8976092 - Flags: review?(philipp) → review-
(Reporter)

Comment 6

7 months ago
> Sorry for the delay. This isn't quite the right patch, as attachments are
> handled separately instead of as promoted props. The events generated there
> are going against spec, there must only be one ATTACH in an AUDIO alarm.

Exactly. And Thunderbird is responsible for generating the second ATTACH field against spec. So we have to fix that behavior instead of ignoring our own mistake later on. I thought marking it as promoted prop would do exactly that. But if there is another proper way to prevent the duplicate, please do so.
(Reporter)

Comment 7

7 months ago
> What I could imagine doing (which would fix this case) is to only throw an
> error at
> https://searchfox.org/comm-central/source/calendar/base/src/calAlarm.js#329
> if the value (and parameters) of the newly added audio alarm is different
> than the value of the already existing alarm. If the value is the same,
> silently drop the newly added alarm.
> 
> Do you think you could look into creating a patch to that effect?

I don't think so. Right now the marked code snipped does not even throw an error. The duplicate ATTACH line silently gets saved into the TB internal database and causes the remote server to throw the error when you try to send the vevent with the duplicate alarm attachment.

If I remember the steps through the debugger from 2 months ago correctly, then the attachment ist first parsed and added to the mAttachments array in
https://searchfox.org/comm-central/source/calendar/base/src/calAlarm.js#542
and then again as separate property to mProperties in the code block of
https://searchfox.org/comm-central/source/calendar/base/src/calAlarm.js#560

Adding ATTACH to the promotedProps prevents the latter. What other way would there be to prevent that the iterator cal.iterate.icalProperty(aComp) return the ATTACH property? Or is the handling of the ATTACH property within the for loop wrong?

Sorry I am no JavaScript developer. I am just putting this together from my understanding of the code.
(Reporter)

Comment 8

4 months ago
Bump.

Any insights how to prevent Thunderbird from creating a second ATTACH entry other than my proposed patch? The lack of cooperation here is annoying. This is open source software. I debugged the issue, I proposed a fix, but there is no reaction by any maintainer for months.
Created attachment 9010040 [details] [diff] [review]
Fix - v2

I'm thinking something like this, though not sure yet if getProperty should just go ahead and return the first attachment/attendee, and setProperty should clear and add the passed attachment/attendee.

Sorry I've been delaying this, I'm trying to keep up with things but sometimes they fall through the cracks. We aren't exactly a large team :)

Can you see if the patch works for you?
(Assignee)

Updated

4 months ago
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(Assignee)

Updated

4 months ago
Flags: needinfo?(code)
(Reporter)

Comment 10

4 months ago
Comment on attachment 9010040 [details] [diff] [review]
Fix - v2

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

Tested with TB 60, Lightning 6.2
The patch fixes the bug.

I did not compile from source or check test_alarm.js in any way.
Attachment #9010040 - Flags: review+
(Reporter)

Comment 11

4 months ago
Side node: TB60, Lightning 6.2 do not throw errors when importing the foobar_NC.ics (attachment 8974103 [details]) anymore. So an easier way to reproduce the bug is:

1. create new empty local calendar
2. import foobar_NC.ics into calendar
2. export the same calendar again

result: 
The Event FOO BAR contains VALARM with ATTACH:Basso twice.


result with patched calAlarm.js from attachment 9010040 [details] [diff] [review] Fix - v2:
The Event FOO BAR contains VALARM with ATTACH:Basso once.
Flags: needinfo?(code)
(Reporter)

Updated

4 months ago
Summary: Cannot change recurrent Event with Apple generated VALARM → VALARM Attachment is read in twice
Version: Lightning 5.4.7 → Lightning 6.2

Comment 12

3 months ago
Please leave the version of the bug to that value it is reported first.
Version: Lightning 6.2 → Lightning 5.4.7
(Reporter)

Comment 13

3 months ago
So when will this be merged into the next release of ANY version?
Comment on attachment 9010040 [details] [diff] [review]
Fix - v2

This needs a review from a calendar peer first, asking MakeMyDay. When r+'d, we can push it to comm-central, uplift it to beta, and once it has some testing in beta we can uplift it to ESR which will put it into Thunderbird 60.
Attachment #9010040 - Flags: review+ → review?(makemyday)

Comment 15

2 months ago
Comment on attachment 9010040 [details] [diff] [review]
Fix - v2

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

Apart fronm the comment below, can you please add a test trying to set/get these properties? r+ with that considered.

::: calendar/base/src/calAlarm.js
@@ +599,5 @@
>          let name = aName.toUpperCase();
>          if (name in this.promotedProps) {
> +            if (this.promotedProps[name] !== true) {
> +                this[this.promotedProps[name]] = aValue;
> +            }

Can you please add an else block with a LOG message here to indicate that the setting hasn't happened?
Attachment #9010040 - Flags: review?(makemyday) → review+
(Reporter)

Comment 16

2 months ago
(In reply to [:MakeMyDay] from comment #15)

> 
> ::: calendar/base/src/calAlarm.js
> @@ +599,5 @@
> >          let name = aName.toUpperCase();
> >          if (name in this.promotedProps) {
> > +            if (this.promotedProps[name] !== true) {
> > +                this[this.promotedProps[name]] = aValue;
> > +            }
> 
> Can you please add an else block with a LOG message here to indicate that
> the setting hasn't happened?

But setting happens a few blocks away. The true value is not an unexpected condition.

Comment 17

2 months ago
If your calling setProperty it effectively a noop for ATTACH or ATTENDEE with the current patch. The true value in the definition of the promotedProps has nothing to do with setting a property value, but is effectively just used as a filter criteria here to spare explicit naming.
Created attachment 9023598 [details] [diff] [review]
Fix - v3

I went with a WARN message, as this case should not happen and we should fix it if it occurs.
Assignee: code → philipp
Attachment #9023598 - Flags: review+
(Assignee)

Updated

2 months ago
Attachment #9010040 - Attachment is obsolete: true
(Assignee)

Updated

2 months ago
Attachment #8976092 - Attachment is obsolete: true
(Assignee)

Updated

2 months ago
Keywords: checkin-needed

Comment 19

2 months ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/0ea2b141befb
Ensure alarm attendees and attachments are not added twice. r=MakeMyDay
Status: ASSIGNED → RESOLVED
Last Resolved: 2 months ago
Keywords: checkin-needed
Resolution: --- → FIXED

Comment 20

2 months ago
The linter doesn't like this:
https://hg.mozilla.org/comm-central/rev/0ea2b141befb24a2436f66928e14ec65ef87d401#l1.52
+            if (this.promotedProps[name] !== true) {
Can you please provide a follow up.
Flags: needinfo?(philipp)
Flags: needinfo?(makemyday)
Created attachment 9023762 [details] [diff] [review]
Linter fix - v1

Sorry about that, this should do it. My linter didn't complain before, looks like I need to find out what is wrong with my setup.
Flags: needinfo?(philipp)
Attachment #9023762 - Flags: review+
(Assignee)

Updated

2 months ago
Status: RESOLVED → REOPENED
Keywords: checkin-needed
Resolution: FIXED → ---

Comment 22

2 months ago
No problem, thanks for the quick fix, I'll land it with the next lot, most likely within two hours.
Flags: needinfo?(makemyday)

Comment 23

2 months ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/a28654291254
Follow-up: fix linting issue. rs=bustage-fix
Status: REOPENED → RESOLVED
Last Resolved: 2 months ago2 months ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 6.7

Comment 24

2 months ago
Philipp, based on comment 14, did you wanted to uplift this or let it ride the trains?
Flags: needinfo?(philipp)

Comment 25

5 days ago

I assume we can consider that this patch will ride the trains.

Flags: needinfo?(philipp)
You need to log in before you can comment on or make changes to this bug.