Closed Bug 410168 Opened 16 years ago Closed 16 years ago

circular reference in timezone references


(Calendar :: Internal Components, defect)

Not set


(Not tracked)



(Reporter: mvl, Assigned: dbo)




(2 files)

(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.

(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)
Blocks: 367456
Attached patch patch v1Splinter Review
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.
Attachment #294855 - Flags: review?(daniel.boelzle)
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.
Flags: blocking-calendar0.8?
We definitely want this for 0.8
Assignee: nobody → mvl
Flags: blocking-calendar0.8? → blocking-calendar0.8+
OS: Mac OS X → All
Hardware: PC → All
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 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).
Attachment #294855 - Flags: review?(daniel.boelzle) → review-
Attached patch patch v2Splinter Review
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.
Attachment #295091 - Flags: review?(mvl)
Comment on attachment 295091 [details] [diff] [review]
patch v2

Attachment #295091 - Flags: review?(mvl) → review+
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.
Assignee: mvl → daniel.boelzle
Checked in on HEAD and MOZILLA_1_8_BRANCH.
Closed: 16 years ago
Resolution: --- → FIXED
Target Milestone: --- → 0.8
You need to log in before you can comment on or make changes to this bug.