Re: CVS: commit tomas_f abi/src/text/fmt/xp fp_Page.cpp

msevior@physics.unimelb.edu.au wrote:
>>--- fp_Page.cpp 7 Jul 2005 14:24:49 -0000 1.184
>>+++ fp_Page.cpp 18 Aug 2005 18:35:04 -0000 1.185
>>@@ -84,6 +84,9 @@
>> m_pOwner = NULL;
>> pDSL->deleteOwnedPage(this);
>> }
>>+
>>+ delete m_pFooter;
>>+ delete m_pHeader;
>> }
>
> Eeek! If this code works 95% of the time then I have left some bad code in
> place for ages.
>
> I'm nervous about this patch though.

I have had another look at this and there is indeed a pontential for
screw up and the memory leak is probably the lesser of the two evils at
this stage so I will revert it (and consider a better way of fixing this).

The basic problem is that in the hdr/ftr code it is not at all clear who
owns what. The fp_HdrFtrShadow object is effectively owned by fp_Page
(it is allocated and stored there, and it is specific to that instance
of it); but the shadow is also cached in fl_HdrFtrSectionLayout's
m_vecPages, and ~fl_HdrFtrSectionLayout is relied on for its
destruction. This is not great from the design point of view, because
you are bound to end up with dangling pointers and/or memory leaks if
the code is later modified by someone like me who is not familiar into
the last detail with the intricacies of the relationship between the
different classes.

Basically, ~fl_HdrFtrSectionLayout has no business to be deleting
pointer that are owned by fp_Page; it should remove the pointers it
caches, and tell fp_Page to delete the object (so that it can NULL the
member). (This is also the reason why the 'delete this' construct is so
bad; it means that a dangling pointer to 'this' is left somewhere;
objects should only be deleted by their owners.)

Tomas

                
___________________________________________________________
To help you stay safe and secure online, we've developed the all new Yahoo! Security Centre. http://uk.security.yahoo.com
Received on Fri Aug 19 10:46:03 2005