314334 - addSubcomponent ownership model problem / crash

Status: RESOLVED FIXED

(Calendar :: Internal Components, defect)

Description

[Dan Mosedale (:dmosedale, :dmose)]

Right now, calIIcalComponent.addSubcomponent can be called with a subcomponent that still has a parent.  addSubcomponent will happily reset the parent on this item, resulting in two parent objects that think they own the subcomponent.  Eventually, one of these objects will free the child, and then the next time the other attempts to access the child, Bad Things ensue.

Multiple possible strategies have been suggested for dealing with this:

a) Make addSubcomponent clone the entire hierarchy that is passed to it before adding.

b) Forcibly reparent the object (ie call icalcomponent_remove_component on the existing component and its current parent)

c) Make removeSubcomponent behave like icalcomponent_remove_component and return the removed object.  Make addSubcomponent throw an exception if passed an object which still has a parent.  Probably also want to add a calIIcalComponent.clone so that callers have a choice of semantics.

At some level, this involves deciding exactly what calIcalComponent interface handles represent in terms of ownership model.

Comment 1

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

addProperty has the same ownership problems. You can see it by defining ICAL_ERRORS_ARE_FATAL in icalerror.h, and subscribing to a calendar with an unmapped property, for example x-wr-calname or calscale.

Comment 2

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Unfortunately, I do not have cycles to work on Calendar stuff these days (just as it's getting to the good part!), so I am a bad owner for these bugs.  To delete the tragically-large chunk of bugspam, search for gregorianabdication.

Comment 3

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

mvl: how do you feel about c) here?

Comment 4

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

option c) sounds like the most flexible, and thus have the least impact on performance. Always cloning the entire tree doesn't sound like a good idea, so haveing the option would be good.

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

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

Comment 6

[Joey Minta]

Nominated by mickey on IRC.  Blocking+ (but maybe solved by bug 314337?)

Comment 7

[Michael Büttner (no reviews TFN)]

taking this one.

Comment 8

[Michael Büttner (no reviews TFN)]

After investigating this issue I would suspect that the ownership model is already implicitely defined by the current implemention. Each calIcalComponent / calIcalProperty is owned by its parent object, because each object frees its child objects if and only if it has no parent, i.e. it is the topmost instance of the hierarchy. Each child object only tracks a reference to its immediate parent in order to avoid cyclic references.

However, this ownership model is not used consistently. calIcalComponent::AddSubcomponent() as well as calIcalComponent::AddProperty() transfer the ownership of the incoming component / property to the parent they're added to. But the underlying libical objects could still belong to some other hierarchy, since those objects have internal references to libical objects, while the wrapper objects (calIcalComponent / calIcalProperty) ignore this fact.

the above proposed solution b) violates the ownership model, since the incoming component / property could belong to some foreign hierarchy. solution c) would require to place additional burden on client code in order to make sure that only toplevel components and properties are added as child objects. this is certainly possible, but i think solution a) is the most stable and relibale one.

one problem where the inconsistent ownership handling shows up is by using unmapped properties. the ics provider first parses the ics stream to generate the calIcalComponent / calIcalProperty objects from. http://lxr.mozilla.org/seamonkey/source/calendar/providers/ics/calICSCalendar.js#218
All unmapped properties are held in a private array. http://lxr.mozilla.org/seamonkey/source/calendar/providers/ics/calICSCalendar.js#240
During doWriteICS() those properties will be added to a newly created component.
http://lxr.mozilla.org/seamonkey/source/calendar/providers/ics/calICSCalendar.js#397
Those properties still belong to the original root component created by ParseICS(). calIcalComponent::AddProperty() happily adds the forgein property to its component (assuming the ownership can be safly transfered). Of course this ends up with the property being freed twice.

Comment 9

[Michael Büttner (no reviews TFN)]

Attachment: patch v1

This patch contains the modifications to calIcalComponent::AddSubcomponent and calIcalComponent::AddProperty in order to implement the above discussed safe transfer of ownership for components and properties.

Comment 10

[Daniel Boelzle [:dbo]]

As we already intensively discussed this inhouse, IMO it is the correct solution: clone and decouple the underlying ical property/component from its XPCOM parent wrapper component when adding. Essentially, this is a copy-on-write.

first r=dbo

Some minor nits:
> -    // XXX like AddSubcomponent, this is questionable
>      calIcalProperty *ical = NS_STATIC_CAST(calIcalProperty *, prop);

Leave this comment, IMO it just states that the cast is questionable: The implementation depends on internals, thus no free implementations of calIICalXXX are allowed. There are lots of those implicit assumptions about incoming calIICalXXX objects. IMO we need to document this in calIICSService.idl, so API users aren't tempted to try a free implementation of some calIICalXXX.

> +		if(ical->mParent)
> +			ical->mProperty = icalproperty_new_clone(ical->mProperty);

remove the tabs.

Comment 11

[Stefan Sitter [:ssitter]]

For your information: Talkback Incident TB22379870K shows the following stack trace for this issue:
  pvl_pop()
  icalproperty_free()
  icalcomponent_free()
  icalcomponent_free()
  calIcalComponent::~calIcalComponent()
  calIcalComponent::Release()
  nsCOMPtr_base::assign_with_AddRef()
  calIcalComponent::AddProperty()

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #8)
> solution c) would require to place additional burden on client code in order 
> to make sure that only toplevel components and properties are added as child 
> objects.  certainly possible, but i think solution a) is the most stable
> and reliable one.

Good point: c) is really just a version of a) which can give better performance at the cost of more API complexity.  We can certainly land a) now, and if this cloning shows up in future profiles, deal with adding adopt{Subcomponent,Property} hooks in another bug. 

Comment 13

[Dan Mosedale (:dmosedale, :dmose)]

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

r=dmose.  If you wish to add comments to calIICSService.idl as suggested by Daniel, rs=dmose for those too.  Thanks for the patch!

Comment 14

[Michael Büttner (no reviews TFN)]

Patch checked in on MOZILLA_1_8_BRANCH and trunk.