Closed Bug 367186 Opened 17 years ago Closed 16 years ago

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

Categories

(Calendar :: Import and Export, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: cmtalbert, Assigned: ssitter)

Details

Attachments

(1 file, 2 obsolete files)

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.
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?
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         }

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
Added to 0.7 common release notes.
Flags: blocking-calendar0.7? → wanted-calendar0.8+
Attached patch add try-catch logic, rev0 (obsolete) — Splinter Review
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)
Attached patch add try-catch logic, rev1 (obsolete) — Splinter Review
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 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+
Stefan, what is the status here? Can the patch be checked in?
Flags: wanted-calendar0.8+ → blocking-calendar0.8+
I'll provide an updated patch within the next days.
(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.
updated patch
Attachment #293287 - Attachment is obsolete: true
Keywords: checkin-needed
Patch checked into HEAD and MOZILLA_1_8_BRANCH.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Target Milestone: --- → 0.8
Attachment #295778 - Flags: review?(daniel.boelzle)
Attachment #295778 - Flags: review?(daniel.boelzle) → review+
You need to log in before you can comment on or make changes to this bug.