Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Memory leak ? #37

Open
nadrino opened this issue Jun 1, 2020 · 23 comments
Open

Memory leak ? #37

nadrino opened this issue Jun 1, 2020 · 23 comments

Comments

@nadrino
Copy link

nadrino commented Jun 1, 2020

I was trying to make my own overlay with this library, when I figured out Tesla made atmosphere crashing at some point.

In fact, I tried to reproduce this crash by providing an empty shell overlay module and loop with open/close. In addition I was able to reproduce this bug with any overlay module.

Do you have the same problem too ?

@HookedBehemoth
Copy link
Contributor

https://github.com/nadrino/SimpleModManager/blob/master/ovl/source/main.cpp
If this is the source code in question:
You are leaking fs sessions.

@HookedBehemoth
Copy link
Contributor

And you missunderstand how ScopeGuard works.
The lambda will be executed as soon as you leave the scope.

So you'll unmount the sdcard filesystem as soon as you leave initServices

@nadrino
Copy link
Author

nadrino commented Jun 2, 2020

Oh sorry I didn't update the repository while I was testing! Thanks a lot for clarrifying what the ScopeGuard was meant to do. In fact I stopped to use it because I couldn't lookup on the SD after the initialization.

Regarding the crash bug, it happend when I used a very minimal version of my program. In fact everything was commented and only the empty shell was running. Also as I said this bug is present for all the modules I have.

@HookedBehemoth
Copy link
Contributor

So you now close the fs session leak and it still happens?

Could you share this minimal version or point to which other overlays you tested and experience the issue.

Also a crash report would be helpful.
/atmosphere/crash_reports

@HookedBehemoth
Copy link
Contributor

Checked on your repo.
You still leak fs sessions.
Remove fsInitialize.

@nadrino
Copy link
Author

nadrino commented Jun 2, 2020

Oh yeah I forgot to remove it in the commit!

Ok, so in fact in seems that it is my mod_browser that is leaking... I don't know why it affected other tweaks tho.

Here you have a piece of my "minimal code":

#define TESLA_INIT_IMPL // If you have more than one file using the tesla header, only define this in the main one
#include <tesla.hpp>    // The Tesla Header
#include <toolbox.h>
#include <mod_browser.h>
#include <sstream>

mod_browser __mod_browser__;


class GameBrowserGui : public tsl::Gui {
public:


  // Called when this Gui gets loaded to create the UI
  // Allocate all elements on the heap. libtesla will make sure to clean them up when not needed anymore
  tsl::elm::Element* createUI() override {
    // A OverlayFrame is the base element every overlay consists of. This will draw the default Title and Subtitle.
    // If you need more information in the header or want to change it's look, use a HeaderOverlayFrame.
    _frame_ = new tsl::elm::OverlayFrame("SimpleModManager", "");

    // A list that can contain sub elements and handles scrolling


    // Return the frame to have it become the top level element of this Gui
    return _frame_;
  }

  // Called once every frame to update values
  void update() override {

  }

  // Called once every frame to handle inputs not handled by other UI elements
  bool handleInput(u64 keysDown, u64 keysHeld, touchPosition touchInput,
    JoystickPosition leftJoyStick, JoystickPosition rightJoyStick) override {

    return false;   // Return true here to singal the inputs have been consumed
  }

private:

  tsl::elm::OverlayFrame* _frame_;

};


class SimpleModManagerOverlay : public tsl::Overlay {
public:

  // libtesla already initialized fs, hid, pl, pmdmnt, hid:sys and set:sys
  void initServices() override {

    fsdevMountSdmc();

    tsl::hlp::ScopeGuard dirGuard([&] {
    });

  }  // Called at the start to initialize all services necessary for this Overlay
  void exitServices() override {

    fsdevUnmountDevice("sdmc");

  }  // Callet at the end to clean up all services previously initialized

  void onShow() override {}    // Called before overlay wants to change from invisible to visible state
  void onHide() override {}    // Called before overlay wants to change from visible to invisible state

  std::unique_ptr<tsl::Gui> loadInitialGui() override {
    return initially<GameBrowserGui>();  // Initial Gui to load. It's possible to pass arguments to it's constructor like this
  }

};


int main(int argc, char **argv) {
  return tsl::loop<SimpleModManagerOverlay>(argc, argv);
}

Here is the crash repport :
report_0000000178d4beb8.bin.zip

Can you tell me how you look into these ? :)

Thanks for the support !

@nadrino
Copy link
Author

nadrino commented Jun 2, 2020

I was able to make Atmosphere crash again with this even more simple piece of code. However I don't have any crash repport in the folder this time.

#define TESLA_INIT_IMPL // If you have more than one file using the tesla header, only define this in the main one
#include <tesla.hpp>    // The Tesla Header
#include <sstream>


class GameBrowserGui : public tsl::Gui {
public:


  // Called when this Gui gets loaded to create the UI
  // Allocate all elements on the heap. libtesla will make sure to clean them up when not needed anymore
  tsl::elm::Element* createUI() override {
    // A OverlayFrame is the base element every overlay consists of. This will draw the default Title and Subtitle.
    // If you need more information in the header or want to change it's look, use a HeaderOverlayFrame.
    _frame_ = new tsl::elm::OverlayFrame("SimpleModManager", "");

    // A list that can contain sub elements and handles scrolling


    // Return the frame to have it become the top level element of this Gui
    return _frame_;
  }

  // Called once every frame to update values
  void update() override {

  }

  // Called once every frame to handle inputs not handled by other UI elements
  bool handleInput(u64 keysDown, u64 keysHeld, touchPosition touchInput,
    JoystickPosition leftJoyStick, JoystickPosition rightJoyStick) override {

    return false;   // Return true here to singal the inputs have been consumed
  }

private:

  tsl::elm::OverlayFrame* _frame_;

};


class SimpleModManagerOverlay : public tsl::Overlay {
public:

  // libtesla already initialized fs, hid, pl, pmdmnt, hid:sys and set:sys
  void initServices() override {



    fsdevMountSdmc();

    tsl::hlp::ScopeGuard dirGuard([&] {
    });

  }  // Called at the start to initialize all services necessary for this Overlay
  void exitServices() override {

    fsdevUnmountDevice("sdmc");

  }  // Callet at the end to clean up all services previously initialized

  void onShow() override {}    // Called before overlay wants to change from invisible to visible state
  void onHide() override {}    // Called before overlay wants to change from visible to invisible state

  std::unique_ptr<tsl::Gui> loadInitialGui() override {
    return initially<GameBrowserGui>();  // Initial Gui to load. It's possible to pass arguments to it's constructor like this
  }

};


int main(int argc, char **argv) {
  return tsl::loop<SimpleModManagerOverlay>(argc, argv);
}

@masagrator
Copy link
Contributor

masagrator commented Jun 2, 2020

Why you want to mount sdmc for all costs, when it's already mounted? You don't need to initialize sdcard in any NRO for hbmenu, so you don't need to do it in overlay.

In status monitor I'm not using any fs function at all, but opening file from sdcard still works.

@nadrino
Copy link
Author

nadrino commented Jun 2, 2020

Oh I see... In fact I added this call because the "tsl::hlp::ScopeGuard". I didn't undertand why I could not reach any file so I added extra call to reopen sdmc

@nadrino
Copy link
Author

nadrino commented Jun 2, 2020

Here is the crash log I had (after a fresh restart) with this version of the minimal code:

#define TESLA_INIT_IMPL // If you have more than one file using the tesla header, only define this in the main one
#include <tesla.hpp>    // The Tesla Header
#include <sstream>


class GameBrowserGui : public tsl::Gui {
public:

  GameBrowserGui(){
    _frame_=nullptr;
  }

  // Called when this Gui gets loaded to create the UI
  // Allocate all elements on the heap. libtesla will make sure to clean them up when not needed anymore
  tsl::elm::Element* createUI() override {
    // A OverlayFrame is the base element every overlay consists of. This will draw the default Title and Subtitle.
    // If you need more information in the header or want to change it's look, use a HeaderOverlayFrame.
    _frame_ = new tsl::elm::OverlayFrame("SimpleModManager", "");

    // A list that can contain sub elements and handles scrolling


    // Return the frame to have it become the top level element of this Gui
    return _frame_;
  }

  // Called once every frame to update values
  void update() override {

  }

  // Called once every frame to handle inputs not handled by other UI elements
  bool handleInput(u64 keysDown, u64 keysHeld, touchPosition touchInput,
    JoystickPosition leftJoyStick, JoystickPosition rightJoyStick) override {

    return false;   // Return true here to singal the inputs have been consumed
  }

private:

  tsl::elm::OverlayFrame* _frame_;

};


class SimpleModManagerOverlay : public tsl::Overlay {
public:

  // libtesla already initialized fs, hid, pl, pmdmnt, hid:sys and set:sys
  void initServices() override {

    tsl::hlp::ScopeGuard dirGuard([&] {
    });

  }  // Called at the start to initialize all services necessary for this Overlay
  void exitServices() override {

  }  // Callet at the end to clean up all services previously initialized

  void onShow() override {}    // Called before overlay wants to change from invisible to visible state
  void onHide() override {}    // Called before overlay wants to change from visible to invisible state

  std::unique_ptr<tsl::Gui> loadInitialGui() override {
    return initially<GameBrowserGui>();  // Initial Gui to load. It's possible to pass arguments to it's constructor like this
  }

};


int main(int argc, char **argv) {
  return tsl::loop<SimpleModManagerOverlay>(argc, argv);
}

01591114629_420000000007e51a.log

@HookedBehemoth
Copy link
Contributor

Having a hard time reproducing it.

Realized that the vsync event never gets closed.
Could you try adding the following line into tsl::gfx::Renderer::exit()
eventClose(&this->m_vsyncEvent);

Compile and run both your overlay and tesla menu with that fix.

@nadrino
Copy link
Author

nadrino commented Jun 2, 2020

Alright, should I add it before "framebufferClose(&this->m_framebuffer);" or after "viExit();"?

@nadrino
Copy link
Author

nadrino commented Jun 2, 2020

This is usually a lot harder to reproduce with an empty shell.

And I don't know if it's this fix or not, but it was a lot harder than before to make it break.

Here are the two crash repports: (there is actually 2 repports when this happend)
01591127213_420000000007e51a.log
01591127212_420000000007e51a.log

@nadrino
Copy link
Author

nadrino commented Jun 2, 2020

Here is a capture of the crash. This video might seem stupid because no user would do that forcing in any case, but when you have heavier ovl loaded its crashed more often.

Alt text

@HookedBehemoth
Copy link
Contributor

You still use release Tesla Menu

@HookedBehemoth
Copy link
Contributor

Recompile that with the same "fix"

@nadrino
Copy link
Author

nadrino commented Jun 2, 2020

Hmm I recompiled it, but the version in the repo says 1.1.1 (Makefile)

Do I just have to build "ovlmenu.ovl" and put it into the "/switch/.overlays/" folder ?

@nadrino
Copy link
Author

nadrino commented Jun 2, 2020

Oh the version which is displayed is the ovlloader! I will try to recompile it

@HookedBehemoth
Copy link
Contributor

Forgot that ovlmenu/tesla menu shows the version of the ovlloader and not it's own version.

Make sure to recompile it.
To elaborate: The bug is in libtesla so every overlay compiled with the old version causes issues.
Only start overlays in testing that have the fix applied.

@nadrino
Copy link
Author

nadrino commented Jun 2, 2020

Ok so with only SMM and the recompiled ovl I was not able to reproduce this bug so far

@nadrino
Copy link
Author

nadrino commented Jun 2, 2020

Here is a video of me trying to make it crash:

Alt text

The "_fix" after SMM version is actually part of ovlmenu's code

@masagrator
Copy link
Contributor

Oh I see... In fact I added this call because the "tsl::hlp::ScopeGuard". I didn't undertand why I could not reach any file so I added extra call to reopen sdmc

Sorry about that. I'm now making new overlay and it looks like also can't access sdcard files without manually mounting sdmc.

@nadrino
Copy link
Author

nadrino commented Jun 5, 2020

I think it's a good thing that we have to mount sdmc in our own module. Such a call take few milliseconds in the start/stop process, and some ovl modules won't need this at all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants