Segmentation Fault when Accessing ListBox Item

Started by therealcain, 20 December 2017, 19:09:06

therealcain

hello,
I'm trying to print out the current selected item of
Code (cpp) Select
tgui::ListBox::Ptr but i keep getting
QuoteSegmentation Fault ( Core Dumped )
here is my way to print:
Code (cpp) Select
std::cout << explorer -> getSelectedItem().toAnsiString() /*error*/ << std::endl; gdb error:
Quote0x00007ffff776a634 in sf::String::String(sf::String const&) () from /usr/lib/x86_64-linux-gnu/libsfml-system.so.2.4
here is backtrace:
Quote
#0  0x00007ffff776a634 in sf::String::String(sf::String const&) () from /usr/lib/x86_64-linux-gnu/libsfml-system.so.2.4
#1  0x00007ffff7462551 in tgui::ListBox::getSelectedItem() const () from /usr/local/lib/libtgui.so.0.7.6
#2  0x000055555556578f in Filesystem::Filesystem(bool)::{lambda()#1}::operator()() const ()
#3  0x000055555556825d in void std::__invoke_impl<void, Filesystem::Filesystem(bool)::{lambda()#1}&>(std::__invoke_other, Filesystem::Filesystem(bool)::{lambda()#1}&) ()
#4  0x000055555556821e in std::__invoke_result<Filesystem::Filesystem(bool)::{lambda()#1}&>::type std::__invoke<Filesystem::Filesystem(bool)::{lambda()#1}&>(std::__invoke_result&&, (Filesystem::Filesystem(bool)::{lambda()#1}&)...) ()
#5  0x00005555555681e6 in void std::_Bind<Filesystem::Filesystem(bool)::{lambda()#1} ()>::__call<void>(std::tuple<>&&, std::_Index_tuple<>) ()
#6  0x00005555555680f3 in void std::_Bind<Filesystem::Filesystem(bool)::{lambda()#1} ()>::operator()<, void>() ()
#7  0x0000555555567f6e in std::_Function_handler<void (), std::_Bind<Filesystem::Filesystem(bool)::{lambda()#1} ()> >::_M_invoke(std::_Any_data const&) ()
#8  0x00007ffff738f262 in tgui::Signal::operator()(unsigned int) () from /usr/local/lib/libtgui.so.0.7.6
#9  0x00007ffff73f20ed in void tgui::SignalWidgetBase::sendSignal<sf::String>(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&&, sf::String) ()
#10 0x00007ffff73efa3c in tgui::Button::leftMouseReleased(float, float) () from /usr/local/lib/libtgui.so.0.7.6
#11 0x00007ffff73748d6 in tgui::Container::handleEvent(sf::Event&) () from /usr/local/lib/libtgui.so.0.7.6
#12 0x00007ffff7374c3d in tgui::Container::leftMouseReleased(float, float) () from /usr/local/lib/libtgui.so.0.7.6
#13 0x00007ffff7410a84 in tgui::ChildWindow::leftMouseReleased(float, float) () from /usr/local/lib/libtgui.so.0.7.6
#14 0x00007ffff73748d6 in tgui::Container::handleEvent(sf::Event&) () from /usr/local/lib/libtgui.so.0.7.6
#15 0x00007ffff737abfd in tgui::Gui::handleEvent(sf::Event) () from /usr/local/lib/libtgui.so.0.7.6
#16 0x000055555555c109 in Window::loop() ()
#17 0x000055555555bcd0 in main ()
what am i doing wrong ?

texus

Could you show the code that you use to create the ListBox?

therealcain

#2
Here is the source of the Header and Implementation:

Filesystem.hpp
Code (cpp) Select
#ifndef _EXPLORER_HPP
#define _EXPLORER_HPP

#include <experimental/filesystem>
#include <fstream>
#include <sstream>

#include "Globals.hpp"

class Filesystem
{
private:
tgui::ChildWindow::Ptr filesystem_window;
tgui::ListBox::Ptr explorer;

#if defined(WIN32) || defined(WIN64)
    std::string path = "C:/Users/";

    #elif __linux__
    std::string path = "/home/";

    #endif

void ImportFilesExplorer();

public:
Filesystem(bool open);
};

#endif


filesystem.cpp
Code (cpp) Select
#include "../headers/Filesystem.hpp"

namespace fs = std::experimental::filesystem;

Filesystem::Filesystem(bool open)
{
// Setup Widgets
filesystem_window = tgui::ChildWindow::create();
filesystem_window -> setSize(300, 400);
filesystem_window -> setPosition(window.getSize().x / 2, window.getSize().y / 2);

tgui::Button::Ptr button;
button = tgui::Button::create();
button -> setPosition(10, 370);
button -> setSize(280, 25);

explorer = tgui::ListBox::create();
explorer -> setPosition(5, 5);

// Open Files in Listbox
Filesystem::ImportFilesExplorer();

tgui::Button::Ptr openDirButton = tgui::Button::create();
tgui::Button::Ptr backDirButton = tgui::Button::create();


// Open or Save
if(open == true)
{
filesystem_window -> setTitle("Open Project Dialog");
button -> setText("Open Project");
explorer -> setSize(290, 325);

openDirButton -> setPosition(10, 340);
openDirButton -> setSize(120, 20);
openDirButton -> setText("Open Folder");

backDirButton -> setPosition(170, 340);
backDirButton -> setSize(120, 20);
backDirButton -> setText("Back Folder");

// button -> connect("pressed", [&](){

// });
}
else if(open == false)
{
filesystem_window -> setTitle("Save Project Dialog");
button -> setText("Save Project");
explorer -> setSize(290, 320);

tgui::Label::Ptr filename = tgui::Label::create();
filename -> setPosition(7, 333);
filename -> setText("Project Name:");

tgui::EditBox::Ptr projectName = tgui::EditBox::create();
projectName -> setPosition(140, 333);
projectName -> setSize(150, 20);

filesystem_window -> add(projectName);
filesystem_window -> add(filename);

// openDirButton -> setPosition(10, 340);
// openDirButton -> setSize(120, 20);
// openDirButton -> setText("Open Folder");

// backDirButton -> setPosition(170, 340);
// backDirButton -> setSize(120, 20);
// backDirButton -> setText("Back Folder");

// button -> connect("pressed", [&](){

// });
}

openDirButton -> connect("pressed", [&](){
// path = path + explorer -> getSelectedItem().toAnsiString(); // ERROR
// ImportFilesExplorer();
std::cout << explorer -> getSelectedItem().toAnsiString() << std::endl; // ERROR
});

// backDirButton -> connect("pressed", [](){

// });

filesystem_window -> add(button);
filesystem_window -> add(explorer);
filesystem_window -> add(openDirButton);
filesystem_window -> add(backDirButton);

gui.add(filesystem_window);
}

void Filesystem::ImportFilesExplorer()
{
explorer -> removeAllItems();

for(auto& p: fs::directory_iterator(path))
{
std::ostringstream oss;

oss << p;
std::string str = oss.str();

str.erase(std::remove(str.begin(), str.end(), '"'), str.end());

size_t found = str.find_last_of("/\\");
        str.erase(str.begin(), str.begin() + found + 1);

if(fs::is_directory(path + str))
{
        #if defined(WIN32) || defined(WIN64)
        str += "\\";

        #elif __linux__
        str.push_back('/');

        #endif
}

if(str[0] != '.')
explorer -> addItem(str);
}
}


i modified to the full backtrace.

texus

I can't reproduce this. It prints the selected item in the terminal for me. Could you try and make a minimal code that I can just copy-paste and run so that I'm certain that we are testing the exact same code?

therealcain

#4
Quote from: texus on 20 December 2017, 19:30:54
I can't reproduce this. It prints the selected item in the terminal for me. Could you try and make a minimal code that I can just copy-paste so that I'm certain that we are testing the exact same code?

can i send you github link in private message to check it out?

texus


therealcain

Quote from: texus on 20 December 2017, 19:32:33
Sure, if its not too hard to build it.

not sure if it's has been sended you...

texus

I received the PM and have been able to reproduce it. I was accidentally using 0.8 earlier which is probably why I couldn't reproduce it before you sent the project.

texus

Your Filesystem object is destroyed immediately after it gets created in the MenuBar class. The lambda that is called on button press accesses "explorer" which is equivalent of writing "this->explorer" in this case, but your "this" object has already been destroyed.

It isn't a proper fix (you should keep your Filesystem class alive), but the code below does work because it no longer uses "this" and instead uses a copy of the listbox.
Code (cpp) Select
openDirButton -> connect("pressed", [e=explorer](){
  std::cout << e -> getSelectedItem().toAnsiString() << std::endl;
});

therealcain

#9
Quote from: texus on 20 December 2017, 20:16:35
Your Filesystem object is destroyed immediately after it gets created in the MenuBar class. The lambda that is called on button press accesses "explorer" which is equivalent of writing "this->explorer" in this case, but your "this" object has already been destroyed.

It isn't a proper fix (you should keep your Filesystem class alive), but the code below does work because it no longer uses "this" and instead uses a copy of the listbox.
Code (cpp) Select
openDirButton -> connect("pressed", [e=explorer](){
  std::cout << e -> getSelectedItem().toAnsiString() << std::endl;
});


Thank you for that, it works just fine.

I cant manage to modify my string path through the lambda.
i keep getting
Quoteterminate called after throwing an instance of 'std::bad_alloc'
  what():  std::bad_alloc
Aborted (core dumped)
when i try to modify it.

Code (cpp) Select
openDirButton -> connect("pressed", [this, e=explorer](){
path = path + e -> getSelectedItem().toAnsiString();
// ImportFilesExplorer();
std::cout << e -> getSelectedItem().toAnsiString() << std::endl;
});

texus

The path variable has the exact same problem as explorer has, that is why I said it wasn't a proper solution. The Filesystem class object has already been destructed by the time the callback occurs, so accessing any member from it causes undefined behavior.

The real problematic code is not inside the Filesystem class, but in Menubar. The code immediately destructs the Filesystem after creating it.
Code (cpp) Select
{
    Filesystem fs(true);
}

therealcain

Quote from: texus on 20 December 2017, 22:04:57
The path variable has the exact same problem as explorer has, that is why I said it wasn't a proper solution. The Filesystem class object has already been destructed by the time the callback occurs, so accessing any member from it causes undefined behavior.

The real problematic code is not inside the Filesystem class, but in Menubar. The code immediately destructs the Filesystem after creating it.
Code (cpp) Select
{
    Filesystem fs(true);
}


yea i got it by now.
i will call it from window loop to keep it alive, and modify it a little bit, to be more efficient.
thank you !

therealcain

Just wanted to say that i managed to fix it and i don't think the issue was with the constructor.

Using shared_ptr with lambdas causing memory leak, so you're need to use weak_ptr.
Here is better explanation: http://floating.io/2017/07/lambda-shared_ptr-memory-leak/

texus

I'm aware that it can cause a memory leak, but that is about the only thing that can go wrong. It won't just cause crashes.
I tend to capture shared pointers in lambdas out of convenience, but for a serious project it should indeed not be done. You won't find any shared pointers captured inside TGUI though, if there are then it would be considered a bug.

The issue was never with the constructor itself, just the lifetime of your object. It is important that you understand when objects will be destroyed and when you can still access things, because segmentation faults like you encountered are really hard to debug, especially when your program becomes larger. And since the issue had nothing to do with TGUI itself, I might not be able to help you next time. Debugging someone else's code is time consuming which is why I usually want a minimal code, as such code would immediately show whether the issue lies in TGUI or not.