305857 - icalComponent-related crashes

Status: RESOLVED FIXED

(Calendar :: General, defect)

Description

[gekacheka]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7.10) Gecko/20050716 Firefox/1.0.6
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20050824 Mozilla Sunbird/0.2+

Sunbird crashes deleting item from file.
Most apparent difference is that this file has timezone and dates that refer to it.



Reproducible: Always

Steps to Reproduce:
1. Save iCalendar text to local file c:/temp/ICSTest.ics (with crlf newlines)
2. File | new calendar | remote | webdav, file:///c:/temp/ICSTest.ics, name ICSTest
3. event "event with timezone" should appear on 20050825
4. select and delete it.

Actual Results:  
Sunbird crashed.
  The instruction at "0x00829843" referenced memory at "0x00000005".  The memory
could not be "read".

Expected Results:  
No crash, and the event deleted.

(Need bugzilla component for ics provider?)

Comment 1

[gekacheka]

Attachment: example ics file that causes crash when you delete its event in sunbird

Comment 2

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

I have seen sunbird crash, when doing operations related to timezones. But i
couldn't find steps to reproduce.
Your testcase aslo worksforme. I can delete the item just fine. no crash. (on linux)

Comment 3

[gekacheka]

Crash deleting event with timezone seems to be occurring during call to
calICSService.serializeToICS() from calICSCalendar.writeICS() at
http://lxr.mozilla.org/mozilla/source/calendar/providers/ics/calICSCalendar.js#231

  At this call, the calICSCalendar has a VTIMEZONE (added from
  this.unmappedComponents), and no other subcomponents.  (Seems
  correct to keep the timezone in unmappedComponents in case it is
  referenced in other unmapped components or properties.)


(Speculating via lxr:

  calIcalComponent::SerializeToICS appears to implement the call.
  http://lxr.mozilla.org/mozilla/source/calendar/base/src/calICSService.cpp#781

  The first thing serializeToICS does is "add timezone bits", calling
  AddTimezoneComponentToIcal.
  http://lxr.mozilla.org/mozilla/source/calendar/base/src/calICSService.cpp#781

  It seems to call icalcomponent_merge_component with (vcalendar, VTIMEZONE).
  http://lxr.mozilla.org/mozilla/source/calendar/base/src/calICSService.cpp#769

  But icalendarcomponent_merge_component takes (vcalendar, VCALENDAR)
 
http://lxr.mozilla.org/mozilla/source/calendar/libical/src/libical/icalcomponent.c#2099

  So maybe assert fails?  Would that appear as crash?)

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

Seems like a plausible explanation to me.  Wanna try putting together a patch
for the calICSService and see if that fixes it?

Comment 5

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

IF that would indeed be the cause of the crash, why does serializing usually
work just fine? Only in very spacial cases, it dies.

Comment 6

[gekacheka]

(In reply to comment #5)
> IF that would indeed be the cause of the crash, why does serializing usually
> work just fine? Only in very spacial cases, it dies.

Are there VTIMEZONE elements?  In my examinations of the ics file, for events
created with the current eventDialog, there hasn't been a VTIMEZONE element. 
Instead all the dates were zulu.  (I have the "store all dates in universal
time" option turned off, so I'm not sure why they are stored in zulu.)

(If you want to test with more than the above file, the item dialog extension in
bug 298102 gets each date in the preferred timezone.  After creating an event
with this item dialog, each of its dates appears with a timezone id in the .ics
file, and a VTIMEZONE element also appears for the timezone.)

Comment 7

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

(In reply to comment #6)
> Are there VTIMEZONE elements?
I tried it with the testcase attached to this bug. No assertion and no crash.
The event is deleted properly.

Comment 8

[Dan Mosedale (:dmosedale, :dmose)]

For some time, I've been seeing lots of crashes when dealing with
icalComponents.     Often, these happen when adding a subcomponent.  Valgrind
info and stack forthcoming.

Comment 9

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: valgrind output for one crash

Once, I was able to catch one of these crashes using valgrind --tool=addrcheck
(I haven't yet been able to reproduce one while running with --tool=memcheck). 
Attaching the relevant output up to the point of the crash.

Comment 10

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #9)
> Created an attachment (id=197701) [edit]
> valgrind output for one crash
> 
> Once, I was able to catch one of these crashes using valgrind --tool=addrcheck
> (I haven't yet been able to reproduce one while running with --tool=memcheck). 
> Attaching the relevant output up to the point of the crash.

Eyeballing that log, it appears that things start going bad somewhat before we
get to the point of actually crashing.  My interpretation of the first valgrind
warning is that somebody must have passed in a corrupt child component to
calIcalComponent::AddSubcomponent.  So the interesting question becomes: where
did this corrupt component originate?  Unless we're seeing generalized
heap-stomping, presumably the component was generated earlier on in
calICSService.cpp.  Shaver, do you have any ideas or speculation on how this
might have happened?

Comment 11

[Dan Mosedale (:dmosedale, :dmose)]

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

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

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

Comment 13

[Dan Mosedale (:dmosedale, :dmose)]

There are a couple of problems here.  One has to do with a bug in calICSService::AddSubcomponent, and the other is apparently a bug in libical having to do with merging in timezone components.

Comment 14

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: work-in-progress, v1

In the current code, AddSubcomponent attempts to reparent the passed in sub-component.  This has the problem that the calendar to which it originally belonged still thinks that it (also) has ownership.  This patch fixes that and adds some debugging and logging code.  Interesting, with this patch applied, the second problem crops up on essentially every calendar modification rather than just occasionally.  I'm on the trail of the second bug.

Comment 15

[Dan Mosedale (:dmosedale, :dmose)]

I've now spun off bug 314334, bug 314337, and bug 314339 about issues relevant to the crashes going on here.  I believe that we can fix the most basic and common cause of this bug by just having the calICSCalendar provider not manually add VTIMEZONE components, at which point we can close this bug and worry about the other issues separately.

Comment 16

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: stop manually adding VTIMEZONEs

Comment 17

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

Comment on attachment 201278 [details] [diff] [review]
stop manually adding VTIMEZONEs

r=mvl

Comment 18

[Dan Mosedale (:dmosedale, :dmose)]

Fix checked in.