580896 - Mozilla sends invalid calendar sometime: event can contain both DTEND and DUR properties simultaneously.

Status: RESOLVED FIXED

(Calendar :: General, defect)

Description

[Irina Arkhipets]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; ru; rv:1.9.1.10) Gecko/20100504 Firefox/3.5.10
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; ru; rv:1.9.2.7) Gecko/20100713 Lightning/1.0b2 Thunderbird/3.1.1

This looks like some kind of regression of #390492 (https://bugzilla.mozilla.org/show_bug.cgi?id=390492).

We have implemented our own CalDav Server and it works OK with Mozilla Sunbird 0.9. 

However, when I am trying to update one instance of recurrent event in Lightening 1.0b2 or Mozilla Sunbird 1.0-beta, I always get "Property [DURATION] is not applicable" validation exception when my Caldav Server parces recieved event calendar.

The problem is that event modification calendar which Lightening sends to me contains both DTEND and DUR properties simultaneously and this is incorrect.

Please, note:

1. I am successfully able to update single events or all the instances of the same recurrency on my Lightening

2. This is not reproducible with Mozilla Sunbird 0.9.

3. I am able to update recurrence instances in Mozilla Home calendar on the same client.


Reproducible: Always

Comment 1

[Irina Arkhipets]

Attachment: Example of the calendar recieved from Sunbird 1.0.

This is an event calendar received from Mozilla 1.0 after the update of one instance from recurrence serie.

Please, note:
Master event contains DUR property only whereas modified instance contains both DUR and DTEND.

Comment 2

[Stefan Sitter [:ssitter]]

Neither Sunbird nor Lightning allow setting a duration yet. I assume the event has been created by your server and has been edited in Sunbird/Lightning later? Could you provide the original sample event that can be used to reproduce the issue?

Comment 3

[Irina Arkhipets]

Yes, you are right. The event was created on the server side and contains the DURATION property.

Here is the sample of the recurring event created on my server:

...
BEGIN:VCALENDAR
PRODID:-//This is a test//EN
VERSION:2.0
CALSCALE:GREGORIAN
BEGIN:VTIMEZONE
TZID:Pacific/Midway
X-TZINFO:Pacific/Midway[2007g/Partial@883612800000]
BEGIN:STANDARD
TZOFFSETTO:-1100
TZOFFSETFROM:-1100
TZNAME:Pacific/Midway(STD)
DTSTART:19691231T230000
END:STANDARD
END:VTIMEZONE
BEGIN:VEVENT
DTSTAMP:20100723T030818Z
UID:7e6d5f84z30948b46z17432b9f
LOCATION:
SUMMARY:TEST Duration event
DTSTART;TZID=Pacific/Midway:20100718T220000
RRULE:FREQ=DAILY;INTERVAL=1
DURATION:PT3600S
ATTENDEE;ROLE=REQ-PARTICIPANT;PARTSTAT=NEEDS-ACTION;DIR="uid:7e6d5f84z30948b46z13199a40";RSVP=FALSE;CUTYPE=INDIVIDUAL;CN=two one:mailto:[e-mail]
ORGANIZER;CN=Service guest for two one:mailto:$[e-mail]
DESCRIPTION:
BEGIN:VALARM
TRIGGER:-PT15M
ACTION:DISPLAY
DESCRIPTION:MM Reminder
END:VALARM
END:VEVENT
END:VCALENDAR
...

Comment 4

[Magnus Melin [:mkmelin]]

(I see some earlier related old work in bug 390492).

Try to modify a repeating event to change the day of one occurrence. It won't work. 

E.g. for Fastmail caldav


<?xml version="1.0" encoding="utf-8"?>
<D:error xmlns:D="DAV:" xmlns:C="urn:ietf:params:xml:ns:caldav">
  <C:valid-calendar-data/>
  <D:responsedescription>Failed iTIP restrictions for DTEND property. The component must not have both DURATION and DTEND</D:responsedescription>
</D:error>

BEGIN:VCALENDAR
PRODID:-//Mozilla.org/NONSGML Mozilla Calendar V1.1//EN
VERSION:2.0
BEGIN:VTIMEZONE
TZID:Europe/Helsinki
BEGIN:DAYLIGHT
TZOFFSETFROM:+0200
TZOFFSETTO:+0300
TZNAME:EEST
DTSTART:19700329T030000
RRULE:FREQ=YEARLY;BYDAY=-1SU;BYMONTH=3
END:DAYLIGHT
BEGIN:STANDARD
TZOFFSETFROM:+0300
TZOFFSETTO:+0200
TZNAME:EET
DTSTART:19701025T040000
RRULE:FREQ=YEARLY;BYDAY=-1SU;BYMONTH=10
END:STANDARD
END:VTIMEZONE
BEGIN:VEVENT
CREATED:20181015T094127Z
LAST-MODIFIED:20181015T094621Z
DTSTAMP:20181015T094621Z
UID:ed1f6001-20e2-47af-9ddf-14bb34ff14b3
SUMMARY:test
RRULE:FREQ=DAILY;UNTIL=20181221T103000Z;BYDAY=MO,TU,WE,TH,FR
CATEGORIES:Meeting
DTSTART;TZID=Europe/Helsinki:20181016T123000
DTEND;TZID=Europe/Helsinki:20181016T124500
TRANSP:OPAQUE
X-MOZ-GENERATION:1
END:VEVENT
BEGIN:VEVENT
CREATED:20181015T094409Z
LAST-MODIFIED:20181015T094621Z
DTSTAMP:20181015T094621Z
UID:ed1f6001-20e2-47af-9ddf-14bb34ff14b3
SUMMARY:meeting
RECURRENCE-ID;TZID=Europe/Helsinki:20181019T123000
CATEGORIES:Meeting
DTSTART;TZID=Europe/Helsinki:20181018T123000
DTEND;TZID=Europe/Helsinki:20181018T124500
DURATION:PT0S
DESCRIPTION:Can we do Thursday this week?
TRANSP:OPAQUE
CLASS:
SEQUENCE:1
END:VEVENT
END:VCALENDAR

Comment 5

[[:MakeMyDay]]

Have you created/modified the series or this occurrence with another UI before or is that with editing the series/occurrence the first time using Lightning?

Comment 6

[Magnus Melin [:mkmelin]]

IIRC the initial entry was created from the fastmail web ui (since lightning trunk was kind of broken at the time).

Comment 7

[[:MakeMyDay]]

Have you calendar.icaljs enabled? I only see this (-> DURATION:PT0S) with libical (which is not the default on Daily).

Comment 8

[Magnus Melin [:mkmelin]]

I have calendar.icaljs set to false atm.

Comment 9

[[:MakeMyDay]]

Attachment: OmittEmptyProperrtiesWhenProvidingIcalComponent-V1.diff

Since this is an libical only issue, this could be fixed in libical but not likely to happen. An alternate approach might to make sure to not add empty properties to an item since I'm not aware of item properties in RfC 5545 that are allowed to be set without a property value.

The attached patch does exactly that but excludes X-Props from the check to not unintentionally remove them. Maybe this could be extended to the parameters as well. Philipp, want do you think about it?

Comment 11

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 9018806 [details] [diff] [review]
OmittEmptyProperrtiesWhenProvidingIcalComponent-V1.diff

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

This seems like a workaround to the situation, but I am wondering why this even happens? Are we adding the duration property, and if so why? I'd prefer a fix for the regression than a workaround, so f- for now. Happy to discuss!

::: calendar/base/src/calEvent.js
@@ +98,3 @@
>          for (let [name, value] of this.properties) {
>              try {
> +                if ((value || name.startsWith("X-")) && !this.eventPromotedProps[name]) {

This would disallow setting a value to 0, I think we should handle that case.

Comment 12

[[:MakeMyDay]]

Attachment: OmittEmptyProperrtiesWhenProvidingIcalComponent-V2.diff

The culprit is that properties are not deleted but just being nulled in the properties container of the item of the item if this is an occurrence [1]. Since setting an endDate triggers a deleteProperty operation for duration [2], the property enumerator end's up with a property with a null value here, where we don't catch that case.

It's not quite clear to me atm why the special handling for property deletions was introduced (this part of the code hasn't changed since the import from cvs), but given it probably was for a reason, adding a handling in the consumer of the enumerator similar to the first patch seems the right thing to do.

Other consumers of the properties enumarator [3] doesn't look affected at the first glance.

[1] https://searchfox.org/comm-central/source/calendar/base/src/calItemBase.js#391 
[2] https://searchfox.org/comm-central/source/calendar/base/src/calEvent.js#199
[3] https://searchfox.org/comm-central/search?q=of+%5B%5E+%5C.%5D%2B%5C.properties&case=false&regexp=true&path=calendar

Comment 13

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 9029201 [details] [diff] [review]
OmittEmptyProperrtiesWhenProvidingIcalComponent-V2.diff

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

I've been postponing this review for too long, so I'm just going to pull the trigger. This still feels like a pretty invasive change given it worked fine before. I'd appreciate if you could add some tests.

Comment 14

[Magnus Melin [:mkmelin]]

This is waiting on tests to land?

Comment 16

[Magnus Melin [:mkmelin]]

Paul, could you drive this over the finishing line?

Comment 17

[Paul Morris [:pmorris]]

Attachment: OmittEmptyProperrtiesWhenProvidingIcalComponent-V3.diff

Here's a rebased version of the patch that applies to current trunk. I edited the comments slightly.

Philipp, Happy to write tests for this. Can you say more about what you had in mind there?

It looks like the triggering use case is creating a recurring event and then trying to change one occurrence of it. So I assume that that's what we should test. This might overlap/converge with the CALDAV refactoring (bug 1546606) and its xpcshell test?

Comment 18

[Philipp Kewisch [:Fallen][📱📆]]

I'd recommend to have tests that cover all branches of the if() that was changed. You should also check the callers to see in which cases these are being passed in, that might inspire for some more tests. I'm guessing there would be a few for unproxied events, proxied events, etc.

Comment 19

[kyluso]

Hi. It sounds like the fix is ready. Will it be released? Thanks.

Comment 20

[Paul Morris [:pmorris]]

Attachment: OmitEmptyPropertiesWhenProvidingIcalComponent-V4.patch

Adds a basic xpcshell test that covers the failure case reported in this bug. There may be other things to test as part of this, but I wanted to go ahead and put this up for review.

Comment 21

[Geoff Lankow (:darktrojan)]

Comment on attachment 9114973 [details] [diff] [review]
OmitEmptyPropertiesWhenProvidingIcalComponent-V4.patch

I'd rather Philipp reviewed this, since he understands it much better than I do, and he's the one asking for tests.

Comment 22

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 9114973 [details] [diff] [review]
OmitEmptyPropertiesWhenProvidingIcalComponent-V4.patch

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

I'm tempted to needinfo dbo here, let's see if he remembers this code from 10+ years ago ;-) https://searchfox.org/comm-central/source/calendar/base/src/calItemBase.js#139

Anyway, I'd appreciate if you could adapt the tests as follows:

::: calendar/base/src/calEvent.js
@@ +95,5 @@
>      for (let [name, value] of this.properties) {
>        try {
> +        // When deleting a property of an occurrence, the property is not deleted
> +        // but instead set to null, so we need to prevent adding those properties.
> +        let wasReset = this.mIsProxy && value === null;

The tests I'd like to see are essentially all branches of this boolean:
* mIsProxy = true, value === null
* mIsProxy = true, value !== null
* mIsProxy = false, value === null
* mIsProxy = false, value !== null

This will likely involve using getOccurrenceFor() to create a proxy on something that isn't actually an exception, modifying properties on it (check before and after), and then calling the icalComponent getter to find out if both parent item and proxy item has the right properties.

Sorry this might be very verbose, but this is the kind of area I think we need to be dead certain on since it can affect a lot of things.

Comment 23

[Paul Morris [:pmorris]]

Attachment: OmitEmptyPropertiesWhenProvidingIcalComponent-V5.patch

Thanks for the additional details. This patch includes those tests.

Comment 24

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 9119304 [details] [diff] [review]
OmitEmptyPropertiesWhenProvidingIcalComponent-V5.patch

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

Yikes, waiting for a while on me, eh? Sorry for the delay, and thank you for going the extra mile!

Comment 25

[Paul Morris [:pmorris]]

Attachment: OmitEmptyPropertiesWhenProvidingIcalComponent-V6.patch

Thanks for the review, should have pinged you about it sooner. Glad to get this one closed out.

This patch just rebases on the current tip. Here's a try run for good measure.
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=7a93bbb2d31af1810e20aa991c733c4385859cf2

Comment 26

[Paul Morris [:pmorris]]

The try run is looking fine, so I'm going ahead and setting the checkin flag.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/3e7f62e8f741
Make sure to not add properties without values to icalcomponents of events and todos. r=Fallen