329737 - JavaScript Error: "triggerProp has no properties"

Status: RESOLVED FIXED

(Calendar :: General, defect)

Description

[Christian Schmidt]

Older instances of Calendar create ICS files that cannot be imported by recent builds, e.g. Lightning 0.1 RC1 or Sunbird 0.3a1. This is a problem when upgrading.

The offending ICS contains something like this:

BEGIN:VEVENT
...
BEGIN:VALARM
X-LIC-ERROR
 ;X-LIC-ERRORTYPE=VALUE-PARSE-ERROR
 :Cant parse as DURATION value in TRIGGER property. Removing entire
 property: -PT-45M
END:VALARM
END:VEVENT
END:VCALENDAR


When I try to import this, Lightning/Sunbird stops with the following error:

[Exception... "'[JavaScript Error: "triggerProp has no properties" {file: "file:///C:/Documents%20and%20Settings/dr.SCHMIDT/Application%20Data/Thunderbird/Profiles/default/guk7tjnq.slt/extensions/%7Be2fda1a4-762b-4020-b5ad-a41df1933103%7D/components/calItemBase.js" line: 556}]' when calling method: [calIEvent::icalComponent]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: file:///C:/Documents%20and%20Settings/dr.SCHMIDT/Application%20Data/Thunderbird/Profiles/default/guk7tjnq.slt/extensions/%7Be2fda1a4-762b-4020-b5ad-a41df1933103%7D/components/calICSCalendar.js :: anonymous :: line 273"  data: yes]

As far as I can see, calendar/base/src/calItemBase.js assumes that the VALARM entry always contains a TRIGGER entry and does not check for nullness.

For compatibility with older Calender builds and for general robustness, I think a missing TRIGGER entry should just be silently ignored or at least not break the import.

Comment 1

[Stefan Sitter [:ssitter]]

Attachment: testcase created with sunbird 0.2

Works for me. This test event has been created using Sunbird 0.2.

I can import this event without problems into Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060307 Mozilla Sunbird/0.3a1+. Alarm is properly scheduled.

Comment 2

[Stefan Sitter [:ssitter]]

Attachment: testcase created with sunbird 0.3a1

Works for me. This test event has been created using Sunbird 0.3a1.

I can import this event without problems into Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060307 Mozilla Sunbird/0.3a1+. Alarm is properly scheduled.

Comment 3

[Christian Schmidt]

Attachment: Testcase containing problematic VALARM section

The two previous testcases do not contain the problematic VALARM section. Not all files generated with old calendar builds have this problem but some do.

I haven't tried to reproduce the problem using an old build, I have just found the offending parts in some old files generated by several of my colleages.

This specific testcase is made in Notepad because I didn't want to upload an authentic 225 KB calendar. But the offending part in the testcase is copied from an authentic calendar file, and the error message is the same when importing the testcase and the authentic file.

Comment 4

[Joey Minta]

Cant parse as DURATION value in TRIGGER property. Removing entire
 property: -PT-45M

That says that the trigger wasn't null.  Rather, it was a completely invalid duration, namely one with 2 negative signs in it.  Would it be possible to create this in older versions by putting a negative number in for the offset?  Also, can you try to say more specifically which old version output this alarm?  How far back on the version chain are we talking?

Comment 5

[Christian Schmidt]

(In reply to comment #4)
> Cant parse as DURATION value in TRIGGER property. Removing entire
>  property: -PT-45M
> 
> That says that the trigger wasn't null.
Yes, appearently the old calendar build failed to parse the DURATION property and instead wrote this informative error message to the output file. And when a new calendar build tries to parse a file containing this error message, it barfs about triggerProp being null.

> Also, can you try to say more specifically which old version output this
> alarm? How far back on the version chain are we talking?
Several builds have been in use in the office. May best guess is either Sunbird 0.2 or the calendar plug-in for Thunderbird found in calendar_windows_20050111.xpi. AFAICS the version/build id isn't written to the ICS file.

Comment 6

[Joey Minta]

OK, here's what I've found from more testing.  It's possible to create this sort of illegal duration in Sunbird 0.2 by putting in a negative number in the alarm offset slot of the event dialog.  However, even 0.2 freaks out with this data, converting it to some seemingly random large number.  I'm somewhat inclined to say that there's nothing we can do here, because
(1) Hacking libical to parse this duration is dangerous
(2) Even Sunbird 0.2 wasn't "compatible" with the data.
(3) Trigger is required for alarms (see below).

On the other hand, it may be somewhat possible to hack in a fix for just the VALARM component.  The problem is that the TRIGGER is the most important part of an alarm.  Without it, we have absolutely no idea when the alarm should fire.  I'm open to suggestions as to what we ought to substitute if no trigger is defined.  My inclination, however, is that this error is an exactly correct response to a completely invalid alarm.  (Note that it doesn't contain a DESCRIPTION or an ACTION either!)

Comment 7

[Christian Schmidt]

> I'm open to suggestions as to what we ought to substitute if no trigger
> is defined.
I don't know what the general approach is when an ICS file contains an error. Rejecting the whole file because of an error in one entry is suboptimal IMO.

I agree that trying to parse the bogus value is overkill. I suggest that the malformed VALARM entry should just be silently ignored.

Comment 8

[Joey Minta]

Attachment: report error, but keep parsing

This patch makes it so that on the off-chance we encounter an alarm that is invalid like this, we report the error to the console, but will keep parsing the rest of the file.

Comment 9

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 216819 [details] [diff] [review]
report error, but keep parsing

Can you add some information to the errormessage, which tells the user which event failed? (the ID will do, I think)
r=mvl with that.

Comment 10

[Joey Minta]

Patch checked in with additional reporting of item's id.