Bug 39995 - GDIMetaFile::ReplaceAction now releases replaced action, while previous implementation didn't
Summary: GDIMetaFile::ReplaceAction now releases replaced action, while previous imple...
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: LibreOffice (show other bugs)
(earliest affected)
Master old -3.6
Hardware: Other All
: medium normal
Assignee: Caolán McNamara
Depends on:
Reported: 2011-08-11 02:20 UTC by Caolán McNamara
Modified: 2011-08-15 04:55 UTC (History)
2 users (show)

See Also:
Crash report or crash signature:


Note You need to log in before you can comment on or make changes to this bug.
Description Caolán McNamara 2011-08-11 02:20:43 UTC

class MetaAction
    sal_uLong                           mnRefCount;
    void                                Duplicate()  { mnRefCount++; }
    void                                Delete() { if ( 0 == --mnRefCount ) delete this; }

i.e. its reference counted...

In the old 3-4 series we had...
    MetaAction*     ReplaceAction( MetaAction* pAction, sal_uLong nAction ) { return (MetaAction*) Replace( pAction, nAction ); }

i.e. there was no effort made within "ReplaceAction" itself to decrement/increment the ref count

which is why we have...

909: rOutMtf.ReplaceAction( pModAct = pAct->Clone(), nCurPos );
910: pAct->Delete();

but now we have...

MetaAction* GDIMetaFile::ReplaceAction(MetaAction* pAction, size_t nAction)                   
 if ( nAction < aList.size() )
   ::std::vector< MetaAction* >::iterator it = aList.begin();
   ::std::advance( it, nAction );
   *it = pAction;
 return pAction;

i.e. we release the old one when we replace it, which looks good, except that the caller of this is *also* releasing the old one because the previous implementation didn't release it.
Comment 1 Caolán McNamara 2011-08-11 02:22:16 UTC
Noticed when trying to reproduce bug #39799
Comment 2 Caolán McNamara 2011-08-11 02:28:29 UTC
Hmm, and GDIMetaFile::ReplaceAction would return the old action previously I believe, now it return the new action
Comment 4 Joseph Powers 2011-08-11 06:12:04 UTC
It looks like something I did to fix a possible memory leek. Your change looks correct. Now we just need to verify that all the callers do the cleanup.

svtools/source/graphic/grfmgr2.cxx (used twice, ok)
svtools/source/graphic/provider.cxx (used once, ok)
vcl/source/gdi/cvtsvm.cxx (used three times, ok)

Thus, everyone does cleanup and your patch is correct.

Sorry about breaking things.
Comment 5 Caolán McNamara 2011-08-11 06:25:38 UTC
The preexisting api isn't great, its glaringly error prone. Ideally we fix that, i.e. remove it, add proper one with different name, but maybe one thing at a time.