1460001 - VALARM Attachment is read in twice

Status: RESOLVED FIXED

(Calendar :: Alarms, defect)

Description

[Ben Greiner]

Attachment: 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

Comment 1

[Ben Greiner]

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.

Comment 2

[Ben Greiner]

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).

Comment 3

[Ben Greiner]

Attachment: tb-calendar-alarm.patch

Comment 4

[Ben Greiner]

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.

Comment 5

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

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?

Comment 6

[Ben Greiner]

> 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.

Comment 7

[Ben Greiner]

> 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.

Comment 8

[Ben Greiner]

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.

Comment 9

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: 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?

Comment 10

[Ben Greiner]

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.

Comment 11

[Ben Greiner]

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.

Comment 12

[[:MakeMyDay]]

Please leave the version of the bug to that value it is reported first.

Comment 13

[Ben Greiner]

So when will this be merged into the next release of ANY version?

Comment 14

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

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.

Comment 15

[[:MakeMyDay]]

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?

Comment 16

[Ben Greiner]

(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

[[:MakeMyDay]]

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.

Comment 18

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Fix - v3

I went with a WARN message, as this case should not happen and we should fix it if it occurs.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/0ea2b141befb
Ensure alarm attendees and attachments are not added twice. r=MakeMyDay

Comment 20

[Jorg K (CEST = GMT+2)]

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.

Comment 21

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: 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.

Comment 22

[Jorg K (CEST = GMT+2)]

No problem, thanks for the quick fix, I'll land it with the next lot, most likely within two hours.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/a28654291254
Follow-up: fix linting issue. rs=bustage-fix

Comment 24

[[:MakeMyDay]]

Philipp, based on comment 14, did you wanted to uplift this or let it ride the trains?

Comment 25

[[:MakeMyDay]]

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