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

Fix crash if no window_context exists #523

Merged
merged 1 commit into from Jun 20, 2017
Merged

Fix crash if no window_context exists #523

merged 1 commit into from Jun 20, 2017

Conversation

simonschmeisser
Copy link
Contributor

#491 introduced a regression:

In TrajectoryVisualization::onInitialize the existance of a window_context is checked and the trajectory_slider_panel_ and trajectory_slider_dock_panel_ are only created if it exists.
However later on these two pointers are assumed to be valid (non-NULL) and not checked any further.

In TrajectoryVisualization::onInitialize the existance of a window_context is checked and the trajectory_slider_panel_ and trajectory_slider_dock_panel_ are only created if it exists.
However later on these two pointers are assumed to be valid (non-NULL) and not checked any further.
@v4hn
Copy link
Contributor

v4hn commented Jun 13, 2017

hey @simonschmeisser,

did you encounter this segfault or is this patch preventive?
I believe @Jntzko implemented this similar to other parts of the moveit plugins.
I'm waiting for his response first.

If this is relevant, we have to cherry-pick it to i/j too.

@simonschmeisser
Copy link
Contributor Author

@v4hn we do encounter a crash when it tries to set the title of the non-existent panel. We use several rviz::Display and one rviz::PropertyTreeWithHelp in our application but don't have a central rviz window and thus the window_context == nullptr

Copy link
Contributor

@v4hn v4hn left a comment

Choose a reason for hiding this comment

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

I see. Makes sense.
Thanks a lot for the patch and I'm sorry for the inconvenience. :/

+1 . Should be picked to i/j.

@v4hn v4hn requested review from davetcoleman and removed request for davetcoleman June 13, 2017 12:38
@v4hn
Copy link
Contributor

v4hn commented Jun 13, 2017

@davetcoleman, @130s please review/merge.
@Jntzko also found a similar situation in the motion planning rviz plugin and will probably create a similar pull request for it later today.

@Jntzko
Copy link
Contributor

Jntzko commented Jun 13, 2017

The related pull request is #525.

@130s
Copy link
Contributor

130s commented Jun 13, 2017

I'm not sure how to reproduce the issue. Assuming from the comment #523 (comment) by @simonschmeisser, it can only be replicated from source code (I don't know on RViz GUI how to set a title on a non-existent panel)? So I leave the decision up to others.

@v4hn
Copy link
Contributor

v4hn commented Jun 20, 2017

merging. We want to have this regression-fix in the next release.

@v4hn v4hn merged commit a9e32a9 into moveit:kinetic-devel Jun 20, 2017
v4hn pushed a commit that referenced this pull request Jun 20, 2017
In TrajectoryVisualization::onInitialize the existance of a window_context is checked and the trajectory_slider_panel_ and trajectory_slider_dock_panel_ are only created if it exists.
However later on these two pointers are assumed to be valid (non-NULL) and not checked any further.
v4hn pushed a commit that referenced this pull request Jun 20, 2017
In TrajectoryVisualization::onInitialize the existance of a window_context is checked and the trajectory_slider_panel_ and trajectory_slider_dock_panel_ are only created if it exists.
However later on these two pointers are assumed to be valid (non-NULL) and not checked any further.
@v4hn
Copy link
Contributor

v4hn commented Jun 20, 2017

cherry-picked to i/j

@simonschmeisser simonschmeisser deleted the fix-no-window_context branch June 23, 2017 11:58
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

Successfully merging this pull request may close these issues.

None yet

4 participants