Skip to content

fix memory leak#166

Closed
yechun1 wants to merge 0 commit intoros2:ros2from
yechun1:ros2
Closed

fix memory leak#166
yechun1 wants to merge 0 commit intoros2:ros2from
yechun1:ros2

Conversation

@yechun1
Copy link
Copy Markdown
Contributor

@yechun1 yechun1 commented Dec 20, 2017

Use shared_ptr to avoid memory leak


QColor color = Qt::gray;
new rviz_rendering::Grid(
std::shared_ptr<rviz_rendering::Grid>(new rviz_rendering::Grid(
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a temporary, so it will be deleted immediately which is definitely not what you want here.

Perhaps this has overlap with #165? @Martin-Idel-SI can you comment on this? It does look like a memory leak, but I don't have enough context atm to know where it should be owned and cleaned up.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No overlap. The "real" grid is held (and properly deleted) in grid_display, which is loaded via pluginlib.

This grid belongs to the example scene (including the sphere) that was displayed with the rendering_example target. I think the whole scene gets loaded and discarded immediately in rviz, so I'm unsure whether we still need it.

} else {
Ogre::RenderSystem * renderSys = ogre_root_->getRenderSystem();
renderSys->createRenderSystemCapabilities();
std::shared_ptr<Ogre::RenderSystemCapabilities>(renderSys->createRenderSystemCapabilities());
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this does not need to be stored. I believe this is a singleton that was returned.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I try to fix the issue, but not sure the fixing is correct or not.
Code scan found leaked storage: renderSys->createRenderSystemCapabilities()

build/rviz_ogre_vendor/ogre-master-ca665a6-prefix/src/ogre-master-ca665a6/OgreMain/include/OgreRenderSystemCapabilities.h
229 RenderSystemCapabilities* GLRenderSystem::createRenderSystemCapabilities() const
230 {
231 RenderSystemCapabilities* rsc = new RenderSystemCapabilities();
444 rsc->addShaderProfile("arbfp1");

// Resource rsc is not freed or pointed-to in function addShaderProfile

Ogre::RenderSystemCapabilities::addShaderProfile(Ogre::String const &) does not free or save its parameter this.
599 void addShaderProfile(const String& profile)
600 {
601 mSupportedShaderProfiles.insert(profile);
602
603 }

//addShaderProfile() does not free or save its parameter.

@wjwwood wjwwood self-assigned this Jan 3, 2018
@wjwwood wjwwood added bug Something isn't working more-information-needed Further information is required labels Jan 3, 2018
@wjwwood wjwwood closed this Jan 8, 2018
@wjwwood wjwwood mentioned this pull request Jan 8, 2018
@wjwwood
Copy link
Copy Markdown
Member

wjwwood commented Jan 8, 2018

So, because you used the ros2 branch on your fork to open the pr, I could not properly update this pr and GitHub closed it automatically. I opened #173 with fixes to my comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working more-information-needed Further information is required

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants