Closed Bug 1754412 Opened 2 years ago Closed 2 years ago

ical.js: invalid date-time value: "2022-12-20T::

Categories

(Calendar :: ICAL.js Integration, defect)

defect

Tracking

(thunderbird_esr102? fixed)

RESOLVED FIXED
104 Branch
Tracking Status
thunderbird_esr102 ? fixed

People

(Reporter: KaiE, Assigned: lasana)

References

Details

(Keywords: regression)

Attachments

(1 file, 2 obsolete files)

I've switched calendar.icaljs on TB 91 and get the following error many times in console:

NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS: [JavaScript Error: "invalid date-time value: "2022-02-14T::"" {file: "resource:///modules/calendar/Ical.jsm" line: 5943}]'[JavaScript Error: "invalid date-time value: "2022-02-14T::"" {file: "resource:///modules/calendar/Ical.jsm" line: 5943}]' when calling method: [calIIcalComponent::getNextProperty] calIteratorUtils.jsm:167
icalProperty resource:///modules/calendar/utils/calIteratorUtils.jsm:167
next self-hosted:1430
setItemBaseFromICS resource:///components/calItemBase.js:920
set icalComponent resource:///modules/CalEvent.jsm:164
run resource:///modules/CalIcsParser.jsm:272

Error: invalid date-time value: "2022-02-15T::" Ical.jsm:5943:13
fromDateTimeString resource:///modules/calendar/Ical.jsm:5943
fromString resource:///modules/calendar/Ical.jsm:5979
decorate resource:///modules/calendar/Ical.jsm:825
_decorate resource:///modules/calendar/Ical.jsm:2981
_hydrateValue resource:///modules/calendar/Ical.jsm:2964
getValues resource:///modules/calendar/Ical.jsm:3132
propertyIterator resource:///components/calICSService.js:481
next self-hosted:1430
getNextProperty resource:///components/calICSService.js:502
icalProperty resource:///modules/calendar/utils/calIteratorUtils.jsm:167
next self-hosted:1430
setItemBaseFromICS resource:///components/calItemBase.js:920
set icalComponent resource:///modules/CalEvent.jsm:164
run resource:///modules/CalIcsParser.jsm:272

Blocks: icaljs

Do you know where is the data is coming from?

Here is a calendar that triggers it, add it as a "calendar on the network" and subscribe to it, then you should see.
https://www.fes-frankfurt.de/abfallkalender/R29ldGhlc3RyLnwxMXw2MDMxMw%3D%3D.ics

The amount of spam this generates on the console is very annoying.

Also reported at ical upstream: https://github.com/mozilla-comm/ical.js/issues/515
and submitted a pull request upstream: https://github.com/mozilla-comm/ical.js/pull/516

Assignee: nobody → kaie
Status: NEW → ASSIGNED

I suggest to take this as a temporary fix, until we get progress upstream.

I'm not seeing the urgency to patch this? It doesn't really affect users? If console errors are annoying you, why not keep the temporary patch in your own tree and leave it at that?

The general question about leniency probably needs a bit more discussion.

I just noticed that the calendar from comment 3 doesn't contain those values!

It apparently only contains correct dates. For example, it contains DTSTART:20220103
Is that an allowed?

Apparently it's internal Ical code that produces the incorrect value, and then fails to parse it.

(In reply to Andrei Hajdukewycz [:sancus] from comment #8)

I'm not seeing the urgency to patch this? It doesn't really affect users?

I just checked again.

It's a real regression, not just cosmetic. We fail to parse, and no events from this calendar are shown.

Keywords: regression

The failing file has entries like:
DTSTART:20220103

According to this post, the file could be considered invalid:
http://microformats.org/discuss/mail/microformats-dev/2007-January/000187.html

because the default type for DTSTART is a full DATE-TIME. While a simple is a valid value for DTSTART (and it's the expected type for VEVENT), apparently the correct way to specify it would be this format:
DTSTART;VALUE=DATE:20220103

The above post mentions that there were misleading examples in old RFCs (which gave the impression that the DTSTART:20220103 format is valid).
Also, the file worked in past Thunderbird versions, with the old C calendar library (libical).

Should we continue to support the simple format for DATE-only values?

Who wants to give opinions regarding my previous comments?

(In reply to Kai Engert (:KaiE:) from comment #12)

Who wants to give opinions regarding my previous comments?

What is blocking finishing up and getting the upstream patch merged? It's not clear to me from reading the PR.

OK, so after discussing this:

  1. It seems like ical.js does not parse DTSTART:20220103 date-only values like this properly.

  2. The upstream patch does not actually fix that underlying issue, but fixes it after the library has already parsed the string and turned it into an (incorrect) value like 20220103::.

Therefore:
Ideally we should fix ical.js. If we can't do that in a timely manner, then maybe it makes sense to put a temporary fix in here.

Assignee: kaie → lasana
Attachment #9283626 - Attachment is obsolete: true
Attachment #9285370 - Attachment is obsolete: true
Target Milestone: --- → 104 Branch

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/116802edf948
Switch ical.js to lenient mode in parser worker. r=darktrojan

Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED

Comment on attachment 9285358 [details]
Bug 1754412 - Switch ical.js to lenient mode in parser worker. r=darktrojan

[Approval Request Comment]
Regression caused by (bug #): ical.js

User impact if declined:
ics files with date-only DTSTART values will fail to parse.

Testing completed (on c-c, etc.): c-c

Risk to taking this patch (and alternatives if risky):
Since this is only turning on lenient parsing in ical.js it should be fairly low risk, any new bug would have already existed in ical.js. That said, it's possible strict parsing was hiding other bugs.

It is fine to let this live on beta 104 for a week or two if necessary, this isn't terribly urgent.

Attachment #9285358 - Flags: approval-comm-esr102?

Comment on attachment 9285358 [details]
Bug 1754412 - Switch ical.js to lenient mode in parser worker. r=darktrojan

[Triage Comment]
Approved for esr102

Attachment #9285358 - Flags: approval-comm-esr102? → approval-comm-esr102+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: