248884 - big calendar (ics more than 64k) is broken

Status: RESOLVED FIXED

(Calendar :: General, defect)

Description

[Artur Polaczynski]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8a2) Gecko/20040628
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8a2) Gecko/20040628

big calendar (ics more than 64k) is broken
file is truncated on update to magical 64kB size



Reproducible: Always
Steps to Reproduce:
1. instal big calendar
2. update


Actual Results:  
file is broken

Expected Results:  
not truncate the file and display all events

Comment 1

[Michiel van Leeuwen (email: mvl+moz@)]

Confirmed.
The problem is in getting the file. When i used it as a local file, all was ok.
(but a bit slow...)

Comment 2

[Michiel van Leeuwen (email: mvl+moz@)]

The data is truncated at
http://lxr.mozilla.org/seamonkey/source/calendar/resources/content/calendarManager.js#689
689              result = String.fromCharCode.apply(this, result);

This is the conversion from an array of octets into a string.

Comment 3

[Mostafa Hosseini]

Suggested workaround:
http://bugzilla.mozilla.org/show_bug.cgi?id=242939#c7

Comment 4

[Michiel van Leeuwen (email: mvl+moz@)]

A better fix would be to use nsIDownloader. All we want is the file to be saved
to disk.
Hmm, now i think about it, we do check if it starts with something that at least
looks like an ical file. We could do that on the file too, but it means
downloading to a temp location etc.
http://lxr.mozilla.org/seamonkey/source/netwerk/base/public/nsIDownloader.idl

Comment 5

[Brendan Eich [:brendan]]

Re: comment 2: SpiderMonkey has a 64K limit on actual argument count in its
bytecode format, and Function.prototype.apply enforces that same limit.

We could have String.fromCharCodeArray but this is really a case where you want
to avoid going through a JS Array of character codes, then back to a JS string.

/be

Comment 6

[Justin Wood (:Callek)]

Re c#4

Perhaps checking [x] data to see if it looks like an iCal file, if it does,
abort and re-pull using the Downloader to a file?  or something similar?

Comment 7

[Michiel van Leeuwen (email: mvl+moz@)]

*** Bug 265162 has been marked as a duplicate of this bug. ***

Comment 8

[gekacheka]

Attachment: calendarManager.js patch getRemoteCalendarText to convert fromCharCode 32k at a time

Patch implementing converting array to string 32k unicode characters at a time
(similar to comment #3).

(To generate a large calendar for testing, download (don't subscribe) several
different calendar files and import them into one, then upload it to a remote
site.  Before this patch, subscribing to a large remote calendar would produce
a local file of only 65535 characters and would not succeed to point of showing
events.  After this patch, it can become full size, and does show the events.)

Comment 9

[simon annear]

I've used the attached patch from comment #8 to successfully be able to download
and use my remote calendar file (sunone calendar) which was not working prior to
the patch.

This was on a Windows workstation, so the bug should be changed to reference all
OS's (rather than just Linux)

Comment 10

[Mostafa Hosseini]

Fixed in CVS. Thanks.

Comment 11

[Mostafa Hosseini]

Comment on attachment 163935 [details] [diff] [review]
calendarManager.js patch getRemoteCalendarText to convert fromCharCode 32k at a time

A note and a question:
N:Still, a better way to fix this is to use nsIDownloader as stated in comment
#4.
Q:Could it be possible to have an incorrect result if the slice happens in the
middle of a UTF8 character which uses more than one byte?

Comment 12

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #11)
> Q:Could it be possible to have an incorrect result if the slice happens in the
> middle of a UTF8 character which uses more than one byte?

Err, this code has always assumed ISO-8859-1, not UTF-8.

Comment 13

[Michiel van Leeuwen (email: mvl+moz@)]

An alternative approach would be to use the news methods introduced in bug
262385. That would require less code changes then nsIDownloader, but will only
work on new builds.

Comment 14

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: use unicodeconverter

Patch to use nsIScriptableUnicodeConverter, wrapped inside try{}, to fallback
to the old method in case it fails.
It should be more robust against utf8 in the calendar file.

Comment 15

[Michiel van Leeuwen (email: mvl+moz@)]

patch checked in. Should really work now. closing bug (again...)

Comment 16

[gekacheka]

Getting javascript warning 

Warning: variable result hides argument
Source File: chrome://calendar/content/calendarManager.js
Line: 881, Column: 19
Source Code:
               var result = unicodeConverter.convertFromByteArray( result,
result.length );

Comment 17

[gekacheka]

Attachment: calendarManager.js patch: remove 'var' in front of 'result'

(patch -p 2 -i file.patch)

Remove 'var' from in front of assignment to result to fix warning.

Comment 18

[Michiel van Leeuwen (email: mvl+moz@)]

patch checked in

Comment 19

[Reed Loden [:reed]]

The bugspam monkeys have been set free and are feeding on Calendar :: General. Be afraid for your sanity!