410168 - circular reference in timezone references

Status: VERIFIED FIXED

(Calendar :: Internal Components, defect)

Description

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

Comment 1

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

(stupid bugzilla)

In trying to find the cause of the increasing memory, I found that this line in calICSService.cpp causes an increase in memory on every load:
433                 comp->AddTimezoneReference(tz);

The problem here is that the timezone has a reference to the calICalComponent, while the component has a reference to the timezone. Circular reference -> never freed, due to reference counting.

http://lxr.mozilla.org/seamonkey/source/calendar/base/src/calTimezoneService.h#77
http://lxr.mozilla.org/seamonkey/source/calendar/base/src/calICSService.h#148


(note: I'm not claiming that this is the one and only problem, as I have lots of other code commented out. there might be other problems as well)

Comment 2

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

Attachment: patch v1

It turned out that there is another step in the circular reference: the calIcalComponent in the timezone is the vtimezone component. The parent of that is the vcalendar component, which has a list of timezones. 

the reference cycle is now: vcalendar -> timezone -> vtimezone -> vcalendar.

But as far as I know, there is no need for the vtimezone to be part of a vcalendar. Thus the cycle can be broken. See the attached patch.

Comment 3

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

Note: the vcalendar component being in the cycle means that the entire calendar is kept in memory, one copy for every reload. Not good...
With the patch, the memory usage seems to stabilize after a few reloads. There might still be a small increase, but it's much less worse.

Comment 4

[Simon Paquet [:sipaq]]

We definitely want this for 0.8

Comment 5

[Daniel Boelzle [:dbo]]

Good catch, mvl.

(In reply to comment #2)
> But as far as I know, there is no need for the vtimezone to be part of a
> vcalendar. Thus the cycle can be broken. See the attached patch.
Unfortunately it is for libical. icaltimezones are tightly coupled with the parent component. If I apply your patch I get lots of assertions like

###!!! ASSERTION: VTIMEZONE has no parent!: 'mParent', file ...calendar/base/src/calICSService.cpp, line 818

Comment 6

[Daniel Boelzle [:dbo]]

Comment on attachment 294855 [details] [diff] [review]
patch v1

I think your general approach is ok, i.e. decoupling the VTIMEZONE from the VCALENDAR component, but this is r- (see last comment).

Comment 7

[Daniel Boelzle [:dbo]]

Attachment: patch v2

One approach would be to create a parent VCALENDAR and embed the cloned VTIMEZONE into that. Another approach would be to tweak calIcalComponent the way that it stores a handle to the cloned icaltimezone (which manages its own copy of the VTIMEZONE component). I prefer (and implemented) the latter in this patch.

Comment 8

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

Comment on attachment 295091 [details] [diff] [review]
patch v2

r=mvl

Comment 9

[Daniel Boelzle [:dbo]]

Looking twice, I haven't switched the lines we discussed on irc, because it doesn't buy us anything. I have to extra-free clonedZoneComp then in case icaltimezone_new() fails.

Comment 10

[Daniel Boelzle [:dbo]]

Checked in on HEAD and MOZILLA_1_8_BRANCH.