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 memory management for PenInputHandler::sequenceStartPage #5433

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tmoerschell
Copy link
Contributor

Fixes #5400

For now, this is just a patch that will prevent a very specific segmentation fault mentioned above from happening. It is only an addition to what already exists, and as such not a complete cure of the root cause. At least the bug described in the issue is solved.

I do intend to look where there may be other places where we should use the new functions as well.
Any comments welcome

But specifically, input on the following points would be appreciated:

  • I created std::shared_ptr<XojPageView> Layout::getPageViewRefAt() alongside XojPageView* Layout::getPageViewAt(). Does that make sense ? Technically, I believe it would be possible at any time that the XournalView (which owns the page views) gets destroyed even if we had just requested the page view. But using a shared_ptr for everything doesn't seem right either.
  • Is it OK to pass just the raw pointer to called functions once it is properly locked in the parent? Some const std::shared_ptr<XojPageView>& would feel more appropriate, but would also require a ton of other changes and look very much out of place.

@tmoerschell
Copy link
Contributor Author

There are a few places where XojPageView*s are stored for later use. In most cases, it is done in such a way that it is not possible to delete the page view during that time (e.g. the latex controller stores the page view that it will be attached to, and we can't delete a page while the dialog is open).

But I found an issue with the geometry tools:
One can cause a segmentation fault with the following action sequence:

  • Add new page
  • Enable the compass or setsquare on that new page
  • Draw something using the tool
  • Undo (ctrl + z) while drawing

Control::resetGeometryTool() isn't called on undo, so the tool continues to receive events, tries to access a deleted page view and throws a segmentation fault.

The available solutions also illustrate what's possible for the rest.

Solution 1: Add a check to InsertDeletePageUndoAction::deletePage() to reset the geometry tool if it was activated on the deleted page. Simple and effective, but it doesn't guarantee that it will be entirely safe now or in the future.

Solution 2: Add checks pretty much everywhere to make sure the page view still exists before using it. This might happen at every event, so it would probably become way too expensive.

Solution 3: Thightly link the deallocation of a page view with the destruction of all its dependent objects. I can imagine either destruction callbacks or a list of pointers to reset at the beginning of the page view's own destructor.

If someone says that solution 1 is ok, I would gladly use just that. But maybe there's a better way?
My solutions feel like a house of cards, a much too heavy bunker, and a ridiculous contraption. It makes me feel bad about my programming skills :`-)

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.

Error signal 11
1 participant