Closed Bug 423172 Opened 12 years ago Closed 12 years ago

Leaks in calRecurrenceInfo

Categories

(Calendar :: Internal Components, defect)

defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: mvl, Assigned: dbo)

References

Details

(Whiteboard: [gdata-0.5])

Attachments

(1 file)

every calendar item with recurrence has a pointer to a calIRecurrenceInfo, while calIRecurrenceInfo has a pointer to the item
http://mxr.mozilla.org/seamonkey/source/calendar/base/public/calIItemBase.idl#177
http://mxr.mozilla.org/seamonkey/source/calendar/base/public/calIRecurrenceInfo.idl#64

That's a reference cycle and leaks. (with it, I see memory useage increase with each reload. with the pointer to the item removed, there is no increase).

This could be solved in two ways:

1: untangle the two by removing the pointer from the recurinfo to the base item. It now needs an extra parameter for getOccurrenceFor and friends: the item for which the occurences are

2: Make the entanglement explicit by implementing both interfaces in one object. No pointers needed, so no reference cycle.

I'm not sure which one is the better solution. Any opinions?

(related: bug 274793)
A third option would be to get rid of the js wrappers like I sketched in bug 274793 comment #6 (using wrappedJSObject). Admittedly a workaround, but solved the mentioned cycles for me. What bothers me a bit about the above options is that we need to modify the API because of a XPCOM/XPConnect weakness IMO.

Another option I'd like to get investigated (but haven't had the time yet) is to store a weak reference at calRecurrenceInfo for the base item. That weak reference is resolvable since the info object belongs to the item.
Agreed that changing the API isn't the nicest thing to do. (Especially because the cycle collector on trunk should already have fixed the problem here. But we can't use trunk...)

If we get rid of the wrappers, it should not be done as the patch that was proposed, which first uses xpcom to create the objects, and then bypasses it by using .wrappedJSObject. The calRecurreceInfo objects should be created directly, without xpcom. That is very similar to solution 2 from comment 0.

Weak references should also work, and I think that's the cleanest solution.
If we don't go with option 2, we should remove the "shortcuts" that exist in calItemBase that call methods of calIRecurrenceInfo. If we do go with option 2, then we obviously will remove them.

I'm not quite sure how weak references work with javascript, but I'd also say its the best solution.
implementing weak references is as simple as adding nsISupportsWeakReference to your QI impl. Anyone who wants to get a weak reference from an object should call
nsISupportsWeakReference.GetWeakReference();

the other side will need to use nsIWeakReference.QueryReferent() to recover a normal interface.

@see http://mxr.mozilla.org/seamonkey/source/xpcom/base/nsIWeakReference.idl
If we go for the weak reference:
From my experience with other component models (e.g. OpenOffice's UNO), I pay quite some extra cycles when hardening a weak reference. We should assure that adding the weak reference doesn't hurt performance.
Flags: wanted-calendar0.9+
I've investigated into the wcap leaks that users have reported on calendar with recurring items. I've run a lot of tests, and the cause (in contrast to e.g. ics calendars with recurring items that don't leak) is that wcap doesn't use the calIcsParser which is loaded in the same js context as calEvent et al.
Creating events and calling modifyException etc on calRecurrenceInfo seems to create cyclic references that cannot be broken by the js engine (like it works e.g. in ics calendar).
One further problem to add to this bug description is that calRecurrenceInfo actually stores overridden/exceptional items which have a reference to their parent, too. We cannot use weak references for those, thus the only way I see is really to intern the whole scope of event/recurrence-info/exceptional-items, getting rid of wrappers. That turned out to work well to solve the wcap leaks.
I am not sure what the clean long term solution would look like yet. Maybe it makes sense to get rid of the parentItem attribute, and moreover make calIRecurrenceInfo free of holding a reference to it's base item. We could pass in the base item when expanding occurrences.
Getting rid of the wrappers using wrappedJSObject, I ran into another problem:
Having a passed in wrapped event object, the following obviously works:
  eventObject instanceof Ci.calIEvent  => true
while this doesn't:
  eventObject.wrappedJSObject instanceof Ci.calIEvent  => false
The latter doesn't check QueryInterface (at least on branch). We have quite some code that checks for interfaces using instanceof; I think we should fix that, because it might happen there's no wrapper involved when calling.
Requesting review. Mvl, if you have time, I'd appreciate your comment on this, too.

I can imagine QueryInterface is more expensive in some situations, thus we should consider to further optimize calUtils' doQueryInterface, maybe install a interface lookup table.

I haven't changed checks for calIDuration/calIDatetime, because we implement those natively, there's for sure a wrapper involved, thus instanceof works for us.
Assignee: nobody → daniel.boelzle
Status: NEW → ASSIGNED
Attachment #331132 - Flags: review?(philipp)
Comment on attachment 331132 [details] [diff] [review]
[checked in] getting rid of wrappers, instanceof helper

Looks good, it would be nice if we could find out if the problem you are working around also exists on trunk, and if so then file/find a bug to fix it in core JS code.
Attachment #331132 - Flags: review?(philipp) → review+
Comment on attachment 331132 [details] [diff] [review]
[checked in] getting rid of wrappers, instanceof helper

Checked in, however I am unsure whether this is already fully fixed.
Attachment #331132 - Attachment description: getting rid of wrappers, instanceof helper → [checked in] getting rid of wrappers, instanceof helper
I think this is fixed, though thorough testing should follow (I can imagine gdata is affected, too). API changes should be covered by follow-up bugs.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → 0.9
Blocks: 274793
(In reply to comment #6)
> I am not sure what the clean long term solution would look like yet. Maybe it
> makes sense to get rid of the parentItem attribute, and moreover make
> calIRecurrenceInfo free of holding a reference to it's base item. We could pass
> in the base item when expanding occurrences.

That's about what I had in mind, but never got around to fully investigate and implement. But it still sounds like a clean solution to me.
Checked in lightning 2008072904 and sunbird 20080728 -> VERIFIED
Status: RESOLVED → VERIFIED
Whiteboard: [gdata-cvs]
Whiteboard: [gdata-cvs] → [gdata-0.5]
You need to log in before you can comment on or make changes to this bug.