397559 - Missing DTSTART generates a cryptic exception (RFC compliant)

Status: NEW

(Calendar :: Internal Components, defect)

Description

[Robert Eden]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.7) Gecko/20070914 Firefox/2.0.0.7
Build Identifier: 2007092504

The following exception was generated.  No exception was generated in 0.5

[Exception... "'[JavaScript Error: "this.startDate has no properties" {file: "file:///C:/Documents%20and%20Settings/edenr/Application%20Data/Thunderbird/Profiles/fh71cu22.default/extensions/%7Be2fda1a4-762b-4020-b5ad-a41df1933103%7D/js/calEvent.js" line: 230}]' when calling method: [calIEvent::icalComponent]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: file:///C:/Documents%20and%20Settings/edenr/Application%20Data/Thunderbird/Profiles/fh71cu22.default/extensions/%7Be2fda1a4-762b-4020-b5ad-a41df1933103%7D/js/calIcsParser.js :: ip_parseString :: line 117"  data: yes]

Reproducible: Always

Steps to Reproduce:
1. load the ICS
2.
3.


Expected Results:  
Either load the record as a all-day event, or a task.
If an error message is given, include the event details so it can be fixed.

Comment 1

[Robert Eden]

Attachment: sampel ICS with issue

Comment 2

[Sebastian Schwieger]

Confirming. I have tested this with recent nightly and with Lightning 0.5. In case there are more events in the calendar, they are shown without an error in v0.5. 0.7pre pops up an error and doesn't display anything. --> Regression

Comment 3

[Stefan Sitter [:ssitter]]

No regression. The attached .ics file doesn't work in Sunbird 0.3.1, Sunbird 0.5 or Sunbird 0.7pre:

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20070213 Sunbird/0.3.1:

Error: [Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIScriptableDateFormat.FormatDate]"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: file:///D:/sunbird-0.3.1.en-US/js/calDateTimeFormatter.js :: formatDateLong :: line 121"  data: no]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8.1.4pre) Gecko/20070614 Sunbird/0.5:

Error: [Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIScriptableDateFormat.FormatDate]"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: file:///D:/sunbird-0.5.en-US.win32/sunbird/js/calDateTimeFormatter.js :: formatDateLong :: line 149"  data: no]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8.1.8pre) Gecko/20070925 Calendar/0.7pre:

Error: this.startDate has no properties
Source File: file:///D:/sunbird/js/calEvent.js  
Line: 230

Comment 4

[cmtalbert]

The Spec has changed, making us think this was valid ICS, but I was wrong.

The spec has changed to say that for VEVENT, DTSTART *is* required.  But, DTEND and DURATION *are not* required.

http://tools.ietf.org/html/draft-ietf-calsify-rfc2445bis-07#page-52

Marking bug invalid to indicate that it is a problem with the source of this ICS.
--> INVALID

Comment 5

[Robert Eden]

re: No regression
This ICS won't load in 0.7pre, but if a 0.5 user upgrades and accesses the ICS via a remote calendar it will appear broken. 

Comment 6

[Sebastian Schwieger]

(In reply to comment #3)
Ok, things seem to be a bit more complicated. Before I go into detail, let me state that I do think it is a major problem as upgrading from Lightning 0.5 to 0.7 will result in dataloss. The attached event has been sent to the reporter by a rather big Airline company, suggesting that more than one people may run into this problem.

Details:
The attached file only contains the bogus event. Add some more random events and then test the following:
1. Lightning 0.5: Subscribe to the file
2. Lightning 0.7pre: Subscribe to the file

Results:
1. All events except the bogus one are displayed. No error message is displayed
2. An error message pops up. No event is displayed at all.

--> dataloss.

Expected result:
While the error message is fine in general, the rest of the file should be parsed.

Comment 7

[Sebastian Schwieger]

I dont think this bug is INVALID just because the event itself is invalid. The bug  here is, that the rest of the file is not parsed.

Comment 8

[Daniel Boelzle [:dbo]]

Attachment: fix, dumping an error to js console

Comment 9

[Daniel Boelzle [:dbo]]

We have to discuss whether ignoring/filtering invalid ics components is really what we want here: We have to keep in mind that this causes dataloss (in a strict sense) when uploading the calendar data.

Comment 10

[Daniel Boelzle [:dbo]]

The question is IMO basically related to bug 334657, too.

Comment 11

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

Comment on attachment 282408 [details] [diff] [review]
fix, dumping an error to js console

>+        for (var subComp = calComp.getFirstSubcomponent("ANY");
>+             subComp; subComp = calComp.getNextSubcomponent("ANY")) {

Long for() lines are kind of hard to read. On first sight it looked to me as if you forgot the closing ")" of the for. I'm not really sure how to make it better though. I guess putting the condition in a separate line may be more readable, but since I don't have a set opinion on this: optional.

Other than that the patch looks ok, r=philipp


In the long run, I personally think we should try to be as forgiving as possible, similar to html vs. xml. We should still be throwing errors to show both vendors and consumers that the file has errors, but nevertheless try best to import the event. Maybe even a dialog at the end showing events that have had errors while importing, but I guess thats advanced stuff.

Comment 12

[Daniel Boelzle [:dbo]]

We possibly need a different API for that, so callers could check whether the data has had errors and present those to the user.

This needs further discussion => not blocking the release.

Comment 13

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

(In reply to comment #11)
> In the long run, I personally think we should try to be as forgiving as
> possible, similar to html vs. xml.

Forgiving might be OK for importing events, but not for subscribing to a calendar. That difference is quite important, because a subscription will overwrite the file later on, meaning that the not-correctly-data is now gone. That's dataloss and should be prevented.
And given that this bug is in the ics provider component, we should not be forgiving here.

Comment 14

[Stefan Sitter [:ssitter]]

Comment on attachment 282408 [details] [diff] [review]
fix, dumping an error to js console

What is the status of this patch? Was it checked in? Waiting for checkin? Waiting for rework?

Comment 15

[Daniel Boelzle [:dbo]]

Comment on attachment 282408 [details] [diff] [review]
fix, dumping an error to js console

We decided against that patch.
The right way is possibly to rework calIcsParser the way that it could reflect whether a parsing error has occured. Based on that, providers could
- notify all items that have been successfully parsed
- take measures in case of parsing errors, e.g. forcing the calendar readOnly, firing onError, ...

Comment 17

[cmtalbert]

I decided that we ought to take a small fix that will remove this specific error (and allow some of our unit tests to pass) in bug 403241.  However, this does open up the much larger question of "What should the ICS parser do with non-standard files" and I think that is what this bug has morphed into addressing.

I'm going to set the dependency on bug 403241 in the meantime to provide a bit of linkage.

Comment 18

[Daniel Boelzle [:dbo]]

(In reply to comment #17)
> I decided that we ought to take a small fix that will remove this specific
> error (and allow some of our unit tests to pass) in bug 403241.
Which one?

Comment 19

[Martin Schröder [:mschroeder]]

Changing summary. Moving to Internal Components because ICS parser issue (not only ICS provider). Adding dependency on ICS parser error tolerance bug.