Access violation error: object is deleted twice

Discussion in 'C++' started by Boogie El Aceitoso, Oct 1, 2003.

  1. Hi,

    The code below produces an access violation error, complaining that an object
    is destroyed twice.

    I don't understand why this happens. Any help would be appreciated.

    --------main.cpp---------------------------------------------------------------------------
    #include "AbstractActivator.h"


    // 3 classes involved:
    // TextDisplayer: Base class for a hierarchy of functors that
    // display a string in a certain way.
    //
    // Activator: Base class for a hierarchy.
    //
    // ActivatorFactory: Base class of a hierarchy of functors
    // taht return a pointer to a std::list<Activator>
    //
    int main()
    {

    // If I don't create any intances of
    // TextDisplayer, I don't get any errors
    TextDisplayer show;


    // ActivatorFactory is a functor that returns
    // a pointer to an stl::list<Activator>
    // Each Activator instance contains a pointer
    // to a TextDisplayer instance. This internal instance
    // is deleted in the Activator destructor.
    ActivatorFactory fact;

    // Activartors is a typedef for stl::list<Activator>
    Activators * acts = fact();
    Activators::iterator i;

    // Call the activate method of all activators
    for(i = acts->begin(); i != acts->end(); ++i)
    {
    (*i).activate();
    }

    // Delete the dynamically created list<Activators>
    // This is were I get into trouble. This calls
    // the destructor of Activator, that tries to delete
    // the internal TextDisplayer. That line creates an
    // error, complaining that that memory area has already
    // been deleted.
    delete acts;

    return 1;
    }

    ----------------end main.cpp---------------------------------------

    ----------abstractactivators.cpp-----------------------------------

    #include <string>
    #include <list>
    #include "AbstractActivator.h"

    // TextDisplayers
    void DialogDisplayer::eek:perator()(const std::string & aMessage)
    {
    //::ShowMessage(AnsiString(aMessage.c_str()));
    }

    // Activators
    bool Activator::activate()
    {
    //Método base solo usado para debug
    //Los específicos deben de tener en cuenta los
    // posibles errores.
    return true;
    }

    Activator::Activator(TextDisplayer * aDisplayer)
    :theDisplayer(aDisplayer)
    {
    //do nothing
    }

    Activator::~Activator()
    {
    delete theDisplayer;
    }


    // Factory
    Activators * ActivatorFactory::eek:perator()(const ActivatorType aType)
    {

    Activators * lst = new Activators;
    Activator act(new TextDisplayer);
    lst->push_back(act);

    return lst;
    }

    ---------end activators.cpp-----------------------------------------------

    --------activators.h-----------------------------------------
    #ifndef ABSTRACT_ACTIVATOR_FLAG
    #define ABSTRACT_ACTIVATOR_FLAG

    #include <string>
    #include <list>





    class TextDisplayer
    {
    public:
    TextDisplayer(){};
    virtual ~TextDisplayer() {};
    virtual void operator() (const std::string & aMessage) {};

    };

    class DialogDisplayer
    :
    TextDisplayer
    {
    public:
    DialogDisplayer():TextDisplayer(){};
    virtual ~DialogDisplayer() {};
    virtual void operator() (const std::string & aMessage);
    };



    class Activator
    {
    public:
    Activator(TextDisplayer * aDisplayer);
    virtual ~Activator();
    virtual bool activate();

    private:
    TextDisplayer* theDisplayer;
    };


    enum ActivatorType
    {
    NullActivator
    };

    typedef std::list<Activator> Activators;

    class ActivatorFactory
    {
    public:
    virtual Activators * operator()(const ActivatorType aType = NullActivator);
    };

    #endif //ABSTRACT_ACTIVATOR_FLAG

    ----------end-------------------------------------------
     
    Boogie El Aceitoso, Oct 1, 2003
    #1
    1. Advertisements

  2. Classic duh problem.

    It's due to this:

    // Factory
    Activators * ActivatorFactory::eek:perator()(const ActivatorType aType)
    {

    Activators * lst = new Activators;
    Activator act(new TextDisplayer);
    lst->push_back(act);

    return lst;
    }

    On destruction of Activator you delete a TextDisplayer. However
    lst->push_back(act) makes a copy of act. act is later destroyed (on
    exit from the method) and you inadvertently just deleted the
    TextDisplayer object you just created. A potential solution is to use
    reference counting. Or another (example below) is simply to add the
    pointer to the list once it's in the list. Yet another is that there are
    other list implementations that do not use the copy constructor at all.


    Try this:

    // Factory
    Activators * ActivatorFactory::eek:perator()(const ActivatorType aType)
    {

    Activators * lst = new Activators;

    lst->push_back(Activator());

    Activators::iterator iter = lst->end();
    -- iter;
    (*iter).SetDisplayer( new TextDisplayer );

    return lst;
    }

    add this method to class Activator

    virtual void SetDisplayer( TextDisplayer* d )
    {
    TextDisplayer* old = theDisplayer;
    theDisplayer = d;
    delete old;
    }
     
    Gianni Mariani, Oct 1, 2003
    #2
    1. Advertisements

  3. Aaarrrrgghhh!
    Do all STL containers do this? If I add a pointer to a dynamically created
    Activator (instead of one in the stack), will it follow the pointer and make a
    copy of the object?

    Maybe I should just add a copy constructor to Activator and TextDisplayer...

    Thanks! :)
     
    Boogie El Aceitoso, Oct 1, 2003
    #3
  4. Boogie El Aceitoso

    Ron Natalie Guest

    Everythying in C++ does this (not just the standard containers). If yoiu want
    to deep copy an object, you need to provide that in the copying operations.
    This is precisely what you must do. Add a copy assignment operator as well.
    Either that or put the data in a class such as vector or string that had the proper
    copying semantics already.
     
    Ron Natalie, Oct 1, 2003
    #4
  5. The vector comment is ambiguous, not sure what you mean.

    If you can't make copies of TextDisplayer then copying the Activator
    would only be portable if you reference counted (or garbage collected in
    some way) the TextDisplayer objects being pointed to.
     
    Gianni Mariani, Oct 1, 2003
    #5
  6. What he means is that instead of storing a pointer to an object, store an
    actual object. Do you really need to store a pointer to a TextDisplayer?

    Two uses of pointers are for C-style arrays and strings,
    which can be almost drop-in replaced with std::vector and std::string,
    respectively. For most other pointer uses, you can use a handle class with
    deep-copying semantics.
    Or deep-copy the TextDisplayer.

    Josh
     
    Josh Sebastian, Oct 1, 2003
    #6
    1. Advertisements

Ask a Question

Want to reply to this thread or ask your own question?

You'll need to choose a username for the site, which only take a couple of moments (here). After that, you can post your question and our members will help you out.