Closed
Bug 410168
Opened 16 years ago
Closed 16 years ago
circular reference in timezone references
Categories
(Calendar :: Internal Components, defect)
Calendar
Internal Components
Tracking
(Not tracked)
VERIFIED
FIXED
0.8
People
(Reporter: mvl, Assigned: dbo)
References
Details
Attachments
(2 files)
1.59 KB,
patch
|
dbo
:
review-
|
Details | Diff | Splinter Review |
5.89 KB,
patch
|
mvl
:
review+
|
Details | Diff | Splinter Review |
Reporter | ||
Comment 1•16 years ago
|
||
(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)
Blocks: 367456
Reporter | ||
Comment 2•16 years ago
|
||
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)
Reporter | ||
Comment 3•16 years ago
|
||
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?
Comment 4•16 years ago
|
||
We definitely want this for 0.8
Assignee: nobody → mvl
Flags: blocking-calendar0.8? → blocking-calendar0.8+
Updated•16 years ago
|
Status: NEW → ASSIGNED
OS: Mac OS X → All
Hardware: PC → All
Assignee | ||
Comment 5•16 years ago
|
||
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
Assignee | ||
Comment 6•16 years ago
|
||
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-
Assignee | ||
Comment 7•16 years ago
|
||
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)
Reporter | ||
Comment 8•16 years ago
|
||
Comment on attachment 295091 [details] [diff] [review] patch v2 r=mvl
Attachment #295091 -
Flags: review?(mvl) → review+
Assignee | ||
Comment 9•16 years ago
|
||
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
Status: ASSIGNED → NEW
Assignee | ||
Comment 10•16 years ago
|
||
Checked in on HEAD and MOZILLA_1_8_BRANCH.
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Updated•16 years ago
|
Target Milestone: --- → 0.8
Assignee | ||
Updated•16 years ago
|
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•