Invalid ics files don't trigger INVALID_TIMEZONE error, times are assumed as floating

RESOLVED FIXED in 1.0b1

Status

Calendar
Internal Components
--
major
RESOLVED FIXED
10 years ago
8 years ago

People

(Reporter: Stefan Sitter, Assigned: dbo)

Tracking

({dataloss, regression})

unspecified
1.0b1
dataloss, regression
Bug Flags:
wanted-calendar1.0 +
in-testsuite ?

Details

(Whiteboard: [l10n])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

10 years ago
Created attachment 304954 [details]
testcase

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.13pre) Gecko/20080221 Calendar/0.8pre

Invalid ics files don't trigger INVALID_TIMEZONE error, times are assumed as floating

Steps to Reproduce:
1. Import or subscribe to an invalid ics file that references a foreign TZID but doesn't contain a corresponding VTIMEZONE component (see attached testcase)

Actual Results:
Import/subscription succeeds without any error message. But the event is not displayed at the correct time. Silently the events timezone was removed and the times were converted to "floating" --> dataloss.

Expected Results:
The user should be informed that the file is invalid. Either the import/subscription must abort or the user must confirm the possible dataloss. (Or get the possibility to adjust the timezone as requested in Bug 391126).
(Assignee)

Updated

10 years ago
Flags: wanted-calendar0.9+
(Assignee)

Comment 1

10 years ago
some more options:
- user is notified and calendar is enforced to be read-only.
- user is notified and we try to repair the file, match the TZIDs to our Olson set

Comment 2

10 years ago
Since bug 406581 was checked in, the test case in attachment 304954 [details] now produces simple unlocalized error messages on the Error Console:
  Error: Timezone "US/Pacific" not found, falling back to floating!
  Error: Timezone "US/Pacific" not found, falling back to floating!
There is no alert telling the user to look there, however.
(Assignee)

Comment 3

10 years ago
I still like the "user is notified and calendar is enforced to be read-only" option, because it prevents dataloss, but still offers to repair the calendar.
There's one specific problem I see with the current code: ICS parsing does not validate. Right now, the TZID parameter is evaluated late, i.e. after ICS has been parsed, actually when stuffing the calEvent objects with ics components.

Comment 4

10 years ago
Created attachment 336196 [details] [diff] [review]
v1 patch: check source field for floating timezone in events and todos

(patch -l -p 1 -i file.patch)

This patch (to javascript calICSParser) checks the timezone of each parsed event start and end date, and of each todo entry and due date (if they exist).  If a timezone is the floating timezone, then check if it was originally floating (unspecified) in the source ics.  If it wasn't, then collect info for an error.  

At the end, writes all the error info to the error console, and notifies the user to look there via nsIAlertService.  (AlertService is used rather than PromptService so that the user can more easily ignore them if they happen to subscribe to remote ics calendars with errors they can't fix.)

The broken events are identified by title and floating date in the error console, so the user can find and fix them.  No error is thrown, since otherwise the events were parsed ok and should be displayed so the user can fix them (and it would take api changes to the parser and import components to communicate both the info for each error and the parsed results to the caller).

(This patch contains localization changes for the error messages and alert, so  review request withheld due to string freeze.)

Updated

10 years ago
Whiteboard: [l10n] [patch in hand]
(Assignee)

Comment 5

10 years ago
Another idea: What about adding the notion of a "phantom" or "incomplete" timezone to calITimezone? We could add an explicit attribute reflecting that there's no definition/component available, or (alternatively) do checks like "tz.tzid != "floating" && tz.component == null) to identify them. I think this would have two advantages:
* we would preserve the TZID which makes it easier for the user to repair the data
* it makes the check/warning phase easier and faster
Comment on attachment 336196 [details] [diff] [review]
v1 patch: check source field for floating timezone in events and todos

This patch serializes any just-parsed ics, and then tries to parse it using regexes. I'm not really a fan of that. (any regexp based parsing without a complete parser scares me)
I like somehting along the line of what daniel suggested a lot better. There must be a time at which the conversion to floating is made. That sounds like the right point to make a note of that conversion, to be able to show a warning later.

Comment 7

10 years ago
(In reply to comment #5)
> Another idea: What about adding the notion of a "phantom" or "incomplete"
> timezone to calITimezone? We could add an explicit attribute reflecting that
> there's no definition/component available, or (alternatively) do checks like
> "tz.tzid != "floating" && tz.component == null) to identify them.

Sounds promising.  One concern would be finding all consumers of timezones and making sure they are adapted for this change in interface behavior.  (Is there enough time before release 0.9?)

(In reply to comment #6)
> (From update of attachment 336196 [details] [diff] [review])
> This patch serializes any just-parsed ics, and then tries to parse it using
> regexes. I'm not really a fan of that. (any regexp based parsing without a
> complete parser scares me)

If it is any comfort:  ics requires content continuation lines start with a space, so since the patterns all match the property name at the start of a line, they can't match content.  The parameter matching permits additional parameters on either size of TZID=.  I guess one possible way it might produce a different result is if there were more than one TZID parameter --- I'm not sure what the parser does in that case.

> I like somehting along the line of what daniel suggested a lot better. There
> must be a time at which the conversion to floating is made. That sounds like
> the right point to make a note of that conversion, to be able to show a warning
> later.

See the aforementioned bug 406581.

Best done by a c++ expert.  I won't be implementing this.
(Assignee)

Comment 8

10 years ago
(In reply to comment #7)
> (In reply to comment #5)
> > Another idea: What about adding the notion of a "phantom" or "incomplete"
> > timezone to calITimezone? We could add an explicit attribute reflecting that
> > there's no definition/component available, or (alternatively) do checks like
> > "tz.tzid != "floating" && tz.component == null) to identify them.
> 
> Sounds promising.  One concern would be finding all consumers of timezones and
> making sure they are adapted for this change in interface behavior.  (Is there

Sure. But I think that check could wired into the ics parser like you've sketched out in the patch. The only difference I see is that we don't need regexp parsing for the check, but would do the above mentioned.
Long term it would be nice to tag lacking timezones in UI, so the user could repair it.

BTW: Another benefit from preserving the TZID is that we could properly roundtrip it (even in case the user hasn't repaired it). There won't be dataloss (in the strong sense).

> enough time before release 0.9?)

No, I don't think this will make 0.9.
(Assignee)

Updated

10 years ago
Flags: wanted-calendar1.0+
Flags: wanted-calendar0.9+
Flags: blocking-calendar1.0?
(Assignee)

Comment 9

10 years ago
Created attachment 341427 [details] [diff] [review]
patch - v1

- Implements the notion of a phantom timezone and performs a check in the ics parser component, notifying errors (like gekacheka suggested in the previous patch).
- Adds cal.isPhantomTimezone() to be used e.g. in the event dialog to signal stale timezones.
- I couldn't test the alerts-service notification, because it's not available on Mac.

This patch introduces resource://calendar/modules/calUtils.jsm being the new vehicle of common helper functions, to avoid name clashes (like we had with SPAMato) in the future.
That module file exports the |cal| object which all helpers are scoped into. I think we should migrate calUtils.js' symbols into that module file over time. To help new code being written NOW, I've loaded all symbols of calUtils.js into |cal|, so we could start using them, e.g. |cal.createDatetime()| instead of |createDatetime()|.
Further thoughts/suggestions/improvements welcome!
Assignee: nobody → daniel.boelzle
Attachment #341427 - Flags: review?(philipp)
Comment on attachment 341427 [details] [diff] [review]
patch - v1

Looks good, our first module :-) r=philipp
Attachment #341427 - Flags: review?(philipp) → review+
(Assignee)

Comment 11

10 years ago
Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/0a454b8f8da7>

-> FIXED
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Flags: blocking-calendar1.0?
Resolution: --- → FIXED
Target Milestone: --- → 1.0
(Assignee)

Updated

10 years ago
Whiteboard: [l10n] [patch in hand] → [l10n]
Attachment #336196 - Attachment is obsolete: true
Flags: in-testsuite?
Target Milestone: 1.0 → 1.0b1
You need to log in before you can comment on or make changes to this bug.