Memory corruption in std::vector

Illy · August 28, 2016, 2:33pm

Hi, I’m tryng to fill a vector with elements of costum class “Circle”, but when I try to execute the constructor of “RaceLight”, which calls the vector’s push_back function, I have a memory corruption error. However, if “RaceLIght” doesn’t inherit from the Juce class “timer”, I don’t have this problem.
Here’s the error and the Source files: , RaceLight and Circle:

Error in `/home/ilaria/TNDSproject/Builds/LinuxMakefile/build/NewProject’: malloc(): memory corruption: 0x0871d528 ***

#ifndef CIRCLE_H_INCLUDED
#define CIRCLE_H_INCLUDED

#include “JuceHeader.h”

class Circle : public Component
{
public:
Circle(unsigned int);
~Circle();

void paint (Graphics&);
void resized();
void SetColour(unsigned int c);

private:

Colour colour;

//=========================================================================

JUCE_DECLARE_NON_COPYABLE_WITH_LEAK_DETECTOR (Circle)

};

#endif // CIRCLE_H_INCLUDED

#include “Circle.h”

//==============================================================================
Circle::Circle(unsigned int c):colour(c)
{
// In your constructor, you should add any child components, and
setSize (600, 400);
// initialise any special settings that your component needs.

}

Circle::~Circle()
{
}

void Circle::paint (Graphics& g)
{
/* This demo code just fills the component’s background and
draws some placeholder text to get you started.

   You should replace everything in this method with your own
   drawing code..
*/

  // clear the background

g.setColour (colour);
g.fillEllipse (0,0,getWidth(),getHeight());

}

void Circle::resized()
{
// This method is where you should set the bounds of any child
// components that your component contains…

}

void Circle::SetColour(unsigned int c)
{
colour=Colour(c);
repaint();
}

class RaceLight : public Component,
private Timer
{
public:
//==============================================================================
RaceLight (int N_lights);
~RaceLight();

//==============================================================================
//[UserMethods]     -- You can add your own custom methods in this section.
void light();
//[/UserMethods]

void paint (Graphics& g);
void resized();

private:
//[UserVariables] – You can add your own custom variables in this section.
int NumberOfLights;
int TimeOfLight=1000;
vector <ScopedPointer> RedLight;
vector <ScopedPointer> GreenLight;
void timerCallback() override;
//[/UserVariables]

//==============================================================================

//==============================================================================
JUCE_DECLARE_NON_COPYABLE_WITH_LEAK_DETECTOR (RaceLight)

};

//[EndFile] You can add extra defines here…
//[/EndFile]

#endif // JUCE_HEADER_531018019E54512

#include “RaceLight.h”

//[MiscUserDefs] You can add your own user definitions and misc code here…
//[/MiscUserDefs]

//==============================================================================
RaceLight::RaceLight (int N_lights)
: NumberOfLights(N_lights)
{
//[Constructor_pre] You can add your own custom stuff here…
RedLight.reserve(NumberOfLights);
GreenLight.reserve(NumberOfLights);
for(int i=0;i<NumberOfLights;++i){
RedLight.push_back(new Circle(0xffb1082a));
GreenLight.push_back(new Circle(0xff09240a));
addAndMakeVisible(RedLight[i]);
addAndMakeVisible(GreenLight[i]);
}

Thank you very much!

martinrobinson-2 · August 28, 2016, 2:56pm

Could you use JUCE OwnedArray<> rather than vector<ScopedPointer> instead?

floating.point · August 29, 2016, 6:03am

You have to give ScopedPointer a template argument:

ScopedPointer<Circle> or ScopedPointer<Component>

martinrobinson-2 · August 29, 2016, 7:57am

I suspect that it is vector<ScopedPointer<Component>> or vector<ScopedPointer<Circle>> in the OP original code as it wasn’t formatted as preformatted text in the post.

The problem is related to this:

I think you must have been “lucky” when it worked the first time. Do something like this:

class MainContentComponent   : public Component
{
public:
    //==============================================================================
    MainContentComponent()
    {
        comps.add (new Slider());
        comps.add (new Slider());
        comps.add (new TextButton ("Button 1"));
        comps.add (new TextButton ("Button 2"));

        for (auto comp : comps)
            addAndMakeVisible (comp);
        
        setSize (600, 400);
    }
    
    ~MainContentComponent()           { }
    void paint (Graphics& g) override { g.fillAll (Colours::grey); }
    
    void resized() override
    {
        Rectangle<int> r = getLocalBounds();
        
        for (auto comp : comps)
            comp->setBounds (r.removeFromTop (30).reduced (4));
    }
    
private:
    OwnedArray<Component> comps;
};

If you want to use std containers then something like this is what you want:

class MainContentComponent   : public Component
{
public:
    //==============================================================================
    MainContentComponent()
    {
        comps.emplace_back (new Slider());
        comps.emplace_back (new Slider());
        comps.emplace_back (new TextButton ("Button 1"));
        comps.emplace_back (new TextButton ("Button 2"));

        for (auto& comp : comps)
            addAndMakeVisible (comp.get());
        
        setSize (600, 400);
    }
    
    ~MainContentComponent()           { }
    void paint (Graphics& g) override { g.fillAll (Colours::grey); }
    
    void resized() override
    {
        Rectangle<int> r = getLocalBounds();
        
        for (auto& comp : comps)
            comp->setBounds (r.removeFromTop (30).reduced (4));
    }
    
private:
    vector<unique_ptr<Component>> comps;
};

Notice:

use emplace_back() — to move the unique_ptr into the vector
notice the auto& in the iterators (athough using operator[] returns a reference too)

Illy · August 29, 2016, 9:50pm

I tried to use your advice but it didn’t work. These are the changes I made:

class RaceLight  : public Component,
               private Timer
{
public:

RaceLight (int N_lights);
~RaceLight();


void light();

void paint (Graphics& g);
void resized();



private:
    int NumberOfLights;
    int TimeOfLight=1000;
    vector <unique_ptr<Circle>> RedLight;
    vector <unique_ptr<Circle>> GreenLight;
    void timerCallback() override;
    
    JUCE_DECLARE_NON_COPYABLE_WITH_LEAK_DETECTOR (RaceLight)
};

    RaceLight::RaceLight (int N_lights)
    : NumberOfLights(N_lights)
{

   for(int i=0;i<NumberOfLights;++i){
        RedLight.emplace_back(new Circle(0xffb1082a));
        GreenLight.emplace_back(new Circle(0xff09240a));
   }
    for(auto& num:RedLight)
        addAndMakeVisible(num.get());
    for(auto& num:GreenLight)
        addAndMakeVisible(num.get());
 
    setSize (600, 200);
}

I tried also OwnedArray but it was the same.

roeland-2 · August 29, 2016, 11:40pm

Well the only thing you know if you have “memory corruption” is that some time earlier, somewhere in your code a memory corruption bug occurred. From the info you posted there’s no way of knowing where and when, and it may be not related to your RaceLight class at all.

I don’t think the ScopedPointer behaves like an std::unique_ptr. Look at the assignment operator: the ScopedPointer has an overload accepting a lvalue reference (ScopedPointer &other), which implements a move instead of an assignment. The unique_ptr only accepts an rvalue (as in unique_ptr &&other) which also implements a move, but that is expected in this case.

My guess is that, since it predates C++11, it behaves like a std::auto_ptr, and it shouldn’t be used in standard containers at all, not even with emplace_back.

martinrobinson-2 · August 30, 2016, 6:27am

Well, if you get the same problem using OwnedArray then the corruption problem is something else.

Could you reformat the code in your original post so it’s preformatted?

Illy · August 30, 2016, 8:21am

Sure!

class Circle    : public Component
{
public:
    Circle(unsigned int);
    ~Circle();

    void paint (Graphics&);
    void resized();
    void SetColour(unsigned int c);


private:
    
    Colour colour;
    
    //=========================================================================
    
    JUCE_DECLARE_NON_COPYABLE_WITH_LEAK_DETECTOR (Circle)
  
};


Circle::Circle(unsigned int c):colour(c)
{
setSize (600, 400);
}

Circle::~Circle()
{
}

void Circle::paint (Graphics& g)
{
   

g.setColour (colour);
g.fillEllipse (0,0,getWidth(),getHeight()); 

}

void Circle::resized()
{}

void Circle::SetColour(unsigned int c)
{
colour=Colour(c);
repaint();
}

`class RaceLight  : public Component,
                   private Timer
{
public:

    RaceLight (int N_lights);
    ~RaceLight();
    void light();
    void paint (Graphics& g);
    void resized();



private:
   
    int NumberOfLights;
    int TimeOfLight=1000;
    vector <unique_ptr<Circle>> RedLight;
    vector <unique_ptr<Circle>> GreenLight;
    void timerCallback() override;
    
    JUCE_DECLARE_NON_COPYABLE_WITH_LEAK_DETECTOR (RaceLight)
};
    RaceLight::RaceLight (int N_lights)
        : NumberOfLights(N_lights)
    {
       
       for(int i=0;i<NumberOfLights;++i){
            RedLight.emplace_back(new Circle(0xffb1082a));
            GreenLight.emplace_back(new Circle(0xff09240a));
       }
        for(auto& num:RedLight)
            addAndMakeVisible(num.get());
        for(auto& num:GreenLight)
            addAndMakeVisible(num.get());
      
        setSize (600, 200);

    }
`

martinrobinson-2 · August 30, 2016, 8:51am

What are you doing in the timerCallback()?

Illy · August 30, 2016, 9:49am

timerCallback and light should light a RedLight’s circle every second and then light all the GreenLight’s circles.

void RaceLight::timerCallback()
        {
            RedLight[NumberLightsOn]->SetColour(0xffff0000);
            ++NumberLightsOn;
            repaint();
            if (NumberLightsOn==NumberOfLights)
                stopTimer();

        }
void RaceLight::light() {
    startTimer(TimeOfLight);
    for (int NumberGreenLightsOn=0; NumberGreenLightsOn<NumberOfLights;++NumberGreenLightsOn){
        GreenLight[NumberGreenLightsOn]->SetColour(0xff008000);

    }
}

Illy · August 30, 2016, 3:26pm

I’ve solved the problem: there was an error in the parent component. Thank you for your help!

LeakingLikeASeive · September 12, 2017, 7:24am

One thing to mention, you should really be using std::make_unique rather than those raw new operators when allocating the std::unique_ptr.
Example:

myVec.emplace_back(std::make_unique<Circle>(args))

martinrobinson-2 · September 12, 2017, 9:52am

…assuming C++14 and later, not C++11

Topic		Replies	Views
Newbie Needs Help w./ Regards Memory Leak General JUCE discussion	2	830	September 30, 2013
Store custom Component into Vector General JUCE discussion	10	1870	April 28, 2015
Leaked Objects help General JUCE discussion	4	485	April 9, 2011
std::vector<juce::Label> Error General JUCE discussion	5	357	November 12, 2022
Fixed-capacity vector utility class? Development	22	1460	October 22, 2018

Memory corruption in std::vector

Purchase

Discover

Learn

Support

About

Events

Memory corruption in std::vector

Related Topics

Purchase

Discover

Learn

Support

About

Events