Migration wizard hangs when importing an empty ICS file from calendar extension

RESOLVED FIXED in 0.8

Status

Calendar
Import and Export
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: cmtalbert, Assigned: Stefan Sitter)

Tracking

unspecified
Bug Flags:
blocking-calendar0.8 +

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

11 years ago
User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.1.1) Gecko/20061204 Firefox/2.0.0.1
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a2pre) Gecko/20070116 Calendar/0.6a1

The migration wizard hangs when it attempts to import an empty file from the calendar extension.  This should simply generate an error and allow the migration wizard to proceed.

Reproducible: Always

Steps to Reproduce:
1. Install Calendar Extension into thunderbird, but do not add anything to its HOME calendar
2. Install Sunbird, launch using a clean profile
3. Migration wizard will appear, and offers to migrate from "Mozilla Calendar"
4. Allow migration wizard to proceed.
Actual Results:  
Gets an error during migration that is caught by the Error Console:
Error: [Exception... "Component returned failure code: 0x804a0100 [calIICSService.parseICS]"  nsresult: "0x804a0100 (<unknown>)"  location: "JS frame :: file:///Volumes/Calendar/Calendar.app/Contents/MacOS/js/calIcsParser.js :: ip_parseString :: line 60"  data: no]
Source File: file:///Volumes/Calendar/Calendar.app/Contents/MacOS/js/calIcsParser.js
Line: 60 

No errors are displayed to the user, the migration wizard's progress bar simply stops progressing and the wizard remains hung.

Expected Results:  
The migration wizard should be able to handle this error under the covers and this should not hang the migration wizard.

If you attempt this with the normal File->Import, it displays the error to the user and does not hang.

Comment 1

10 years ago
Since Mozilla Calendar 0.2 evidently used empty files for empty calendars, this should not be an error during migration; the empty calendar should just be silently imported as a calendar with no events in it.

I ran into this same problem when migrating from Thunderbird 1.5 with Mozilla Calendar 0.2 to Thunderbird 2.0 with Lightning 0.5 -- as a result, half of my calendars weren't migrated, and I had to recreate them by hand.
Flags: blocking-calendar0.7?

Comment 2

10 years ago
Thats the relevant file for migration:

http://lxr.mozilla.org/mozilla1.8/source/calendar/base/content/migration.js#603

(Function importICSToStorage)

Thats what is used in import-export.js:

101         try
102         {
103            inputStream.init( fp.file, MODE_RDONLY, 0444, {} );
104 
105            var items = importer.importFromStream(inputStream, {});
106            inputStream.close();
107         }
108         catch(ex)
109         {
110             switch (ex.result) {
111                 case Components.interfaces.calIErrors.INVALID_TIMEZONE:
112                     showError(calGetString("calendar", "timezoneError", [filePath] , 1));
113                     break;
114                 default:
115                     showError(calGetString("calendar", "unableToRead") + filePath + "\n"+ ex);
116             }
117         }
118 
119         if (aCalendar) {
120             putItemsIntoCal(aCalendar, items);
121             return;
122         }

(Reporter)

Comment 3

10 years ago
Unfortunately we're too far into 0.7 to fix it at this point.  We're going to have to just release note this one. But we'll fix it in 0.8.

Keywords: relnote

Comment 4

10 years ago
Added to 0.7 common release notes.

Updated

10 years ago
Flags: blocking-calendar0.7? → wanted-calendar0.8+
(Assignee)

Comment 5

10 years ago
Created attachment 293285 [details] [diff] [review]
add try-catch logic, rev0

Patch adds the fix as suggested in Comment #2.

During testing I noticed that the calendars created by the migration wizard are not visible in the calendar list although the creation was successful. An additional application restart is required to make them visible.
Assignee: nobody → ssitter
Status: NEW → ASSIGNED
Attachment #293285 - Flags: review?(daniel.boelzle)
(Assignee)

Comment 6

10 years ago
Created attachment 293287 [details] [diff] [review]
add try-catch logic, rev1

Mvl suggested on IRC to move the close() into a finally block. This patch iteration does this and adds the same change to import-export.js.
Attachment #293285 - Attachment is obsolete: true
Attachment #293287 - Flags: review?(daniel.boelzle)
Attachment #293285 - Flags: review?(daniel.boelzle)

Comment 7

10 years ago
Comment on attachment 293287 [details] [diff] [review]
add try-catch logic, rev1

looks good, thanks for the patch!

just some nits:
>Index: mozilla/calendar/base/content/migration.js
...
>+        try {
>+            inputStream.init(icsFile, MODE_RDONLY, 0444, {});
>+            var items = icsImporter.importFromStream(inputStream, {});
I like variables to be declared on the same level where used, i.e. outside the try-block. Init with [] so we always have a valid array, especially in case of an exception.

>+        } finally {
>+           inputStream.close();
>+        }
Does close() work in case an exception has occurred executing init()?

>Index: mozilla/calendar/base/content/import-export.js
similar nits in this file

r=dbo
Attachment #293287 - Flags: review?(daniel.boelzle) → review+

Comment 8

10 years ago
Stefan, what is the status here? Can the patch be checked in?

Updated

10 years ago
Flags: wanted-calendar0.8+ → blocking-calendar0.8+
(Assignee)

Comment 9

10 years ago
I'll provide an updated patch within the next days.
(Assignee)

Comment 10

10 years ago
(In reply to comment #7)
> Does close() work in case an exception has occurred executing init()?

I assume it's safe to call but don't found any hint in the .idl descriptions. See <http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/mail/extensions/newsblog/content/feed-subscriptions.js&rev=1.21&mark=943-952#932> for a similar usage.
(Assignee)

Comment 11

10 years ago
Created attachment 295778 [details] [diff] [review]
add try-catch logic, rev2

updated patch
Attachment #293287 - Attachment is obsolete: true
(Assignee)

Updated

10 years ago
Keywords: checkin-needed
Patch checked into HEAD and MOZILLA_1_8_BRANCH.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Keywords: checkin-needed, relnote
Resolution: --- → FIXED
Target Milestone: --- → 0.8

Updated

10 years ago
Attachment #295778 - Flags: review?(daniel.boelzle)

Updated

10 years ago
Attachment #295778 - Flags: review?(daniel.boelzle) → review+
You need to log in before you can comment on or make changes to this bug.