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

Fixed potential crashes caused by dangling pointers. #262

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

LEChaney
Copy link

In the current version, mDragWidget is a raw pointer, which can cause crashes if the widget it points is removed. mDragWidget is set on mouse click, and if the same click also removes the widget, a crash occurs when the mouse is released due to a dangling pointer.

@LEChaney LEChaney changed the title Fixed potential crashes caused by dangling pointer. Fixed potential crashes caused by dangling pointers. Sep 19, 2017
@wjakob
Copy link
Owner

wjakob commented Sep 19, 2017

I feel that there are a few redundant changes here. Why doesn't using ref<> alone fix the issue?

@wjakob
Copy link
Owner

wjakob commented Sep 19, 2017

The if (mDragActive && mDragWidget->getRefCount() == 1) { logic in particular looks suspect/undesirable.

@LEChaney
Copy link
Author

Yes, ref alone does fix the issue. Ideally, these kinds of dangling pointers should probably be resolved by implementing some kind of weak reference, so I was trying to kind of lazily fudge the same functionality here since ref doesn't support weak references. The only issue with using ref alone is that the object is kept alive by the mDragWidget, even though it becomes invisible to the user. I thought this might perhaps cause unexpected behavior in some rare circumstances. In my tests, I found that the Widgets weren't destroyed until I released the mouse button without the extra checks. It seems like mDragWidget triggers an event on itself before it is finally destroyed, so I wanted to avoid this.

@LEChaney
Copy link
Author

I can certainly remove the extra checks if you would prefer. I also copied the mFocus dangling pointer fix from #198, but without any of the extra changes.

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.

2 participants