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.

Sign up for GitHub

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

Jump to bottom

scide: Fix a dangling pointer when removing splits #4645

Merged
merged 5 commits into from
Nov 28, 2019
Merged

scide: Fix a dangling pointer when removing splits #4645

merged 5 commits into from
Nov 28, 2019

Conversation

nhthn
Copy link
Contributor

@nhthn nhthn commented Nov 17, 2019 •
edited
Loading

Purpose and Motivation

Fixes #4133, supersedes #4363. When a GenericCodeEditor is destroyed but a Document refers to it via
mLastActiveEditor, said pointer is dangling and can cause a crash. For
example, creating a split, removing all splits, and then recompiling the
class library can cause a crash.

This commit is a quick bandage, simply setting mLastActiveEditor to a
null pointer in the GenericCodeEditor's destructor. A more thorough
solution would be a stack of the most recently active editors, but the
point is to just prevent an IDE crash right now.

Types of changes

  • Bug fix

To-do list

  • This PR is ready for review

scide: Fix a dangling pointer: Document::mLastActiveEditor
aaa776d 
When a GenericCodeEditor is destroyed but a Document refers to it via
mLastActiveEditor, said pointer is dangling and can cause a crash. For
example, creating a split, removing all splits, and then recompiling the
class library can cause a crash.

This commit is a quick bandage, simply setting mLastActiveEditor to a
null pointer in the GenericCodeEditor's destructor. A more thorough
solution would be a stack of the most recently active editors, but the
point is to just prevent an IDE crash right now.
@nhthn nhthn added bug Issues that relate to unexpected/unwanted behavior. Don't use for PRs. env: SCIDE labels Nov 17, 2019
@nhthn nhthn added this to the 3.10.4 milestone Nov 17, 2019
This was referenced Nov 17, 2019
Closed
Closed
@mossheim
Copy link
Contributor

thanks! two more things to make this robust:

  1. explicitly deleting all of copy assign, copy ctor, move assign, and move ctor for gce, with a note that any implementation would need to also update that pointer. this is typical "rule of 5"-- if you find yourself writing a custom move, copy, or dtor operation, you should probably be doing something custom with the others too. this will help to catch any places in the current code that expect the gce to always live at a stable memory address. i think copying a gce /might/ be okay, but i'd rather not think about it if we don't have to.

  2. gce should also remove the reference to itself if its current doc is changed -- i believe there's at least one place where that happens right now.

@nhthn
Copy link
Contributor Author

nhthn commented Nov 17, 2019 •
edited
Loading

awesome, thanks for the info -- didn't know about Rule of 5

gce should also remove the reference to itself if its current doc is changed -- i believe there's at least one place where that happens right now.

i can't find a reassignment of mDoc in either GenericCodeEditor or ScCodeEditor. if mDoc is in fact constant, then i can just make it a const and add a note about this.

edit: making mDoc a const pointer works fine, so i have done so

@mossheim
Copy link
Contributor

awesome, thanks for the info -- didn't know about Rule of 5

https://en.cppreference.com/w/cpp/language/rule_of_three

i can't find a reassignment of mDoc in either GenericCodeEditor or ScCodeEditor.

aha, can you delete the misleading declaration of unimplemented setDocument(Document*) from the header then? that's probably what made me think that.

making mDoc a const pointer works fine, so i have done so

nice!

mossheim
mossheim reviewed Nov 17, 2019
View reviewed changes
Copy link
Contributor

@mossheim mossheim left a comment

Choose a reason for hiding this comment

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

haven't tested it yet but code looks good! thanks

editors/sc-ide/widgets/code_editor/editor.hpp Show resolved Hide resolved
@joshpar joshpar self-requested a review November 24, 2019 18:32
mossheim
mossheim suggested changes Nov 27, 2019
View reviewed changes
Copy link
Contributor

@mossheim mossheim left a comment

Choose a reason for hiding this comment

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

Tested this with all reproducers. After the code change above I am ready to approve this.

@nhthn nhthn force-pushed the topic/last-active-editor branch from 5e59a0d to c8dbec7 Compare November 28, 2019 03:20
@nhthn
Copy link
Contributor Author

nhthn commented Nov 28, 2019

sweet, thanks

@nhthn nhthn merged commit 23ae93f into supercollider:3.10 Nov 28, 2019
@nhthn nhthn deleted the topic/last-active-editor branch November 28, 2019 20:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mossheim mossheim mossheim approved these changes

@joshpar joshpar Awaiting requested review from joshpar

Assignees
No one assigned
Labels
bug Issues that relate to unexpected/unwanted behavior. Don't use for PRs. env: SCIDE
Projects
None yet
Milestone
3.10.4
Development

Successfully merging this pull request may close these issues.
2 participants
@nhthn @mossheim