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

Status: RESOLVED FIXED

(Calendar :: ICAL.js Integration, defect)

Description

[Kai Engert [:KaiE:]]

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

Comment 1

[Magnus Melin [:mkmelin]]

Do you know where is the data is coming from?

Comment 2

[Magnus Melin [:mkmelin]]

Hmm, https://github.com/mozilla-comm/ical.js/pull/412 and bug 1626739 did make use the lenient mode.

Comment 3

[Kai Engert [:KaiE:]]

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

Comment 4

[Kai Engert [:KaiE:]]

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

Comment 5

[Kai Engert [:KaiE:]]

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

Comment 6

[Kai Engert [:KaiE:]]

Attachment: Bug 1754412 - Early landing of patch suggested upstream to fall back to short date parsing. r=lasana

Comment 7

[Kai Engert [:KaiE:]]

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

Comment 8

[Andrei Hajdukewycz [:sancus]]

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.

Comment 9

[Kai Engert [:KaiE:]]

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.

Comment 10

[Kai Engert [:KaiE:]]

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

Comment 11

[Kai Engert [:KaiE:]]

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?

Comment 12

[Kai Engert [:KaiE:]]

Who wants to give opinions regarding my previous comments?

Comment 13

[Andrei Hajdukewycz [:sancus]]

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

Comment 14

[Lasana Murray]

Attachment: Bug 1754412 - Switch ical.js to lenient mode in parser worker. r=darktrojan

Comment 15

[Lasana Murray]

Attachment: Bug 1754412 - Update ical.js with PR 519. r=#thunderbird-reviewers

https://github.com/mozilla-comm/ical.js/pull/519

Depends on D151750

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/116802edf948
Switch ical.js to lenient mode in parser worker. r=darktrojan

Comment 17

[Andrei Hajdukewycz [:sancus]]

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.

Comment 18

[Wayne Mery (:wsmwk)]

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

[Triage Comment]
Approved for esr102

Comment 19

[Rob Lemley [:rjl]]

Thunderbird 102.1.1:
https://hg.mozilla.org/releases/comm-esr102/rev/77f09e458039