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 fullscreen editor's myScrollBar delayed destruction causing a crash #503

Merged
merged 1 commit into from Feb 25, 2019

Conversation

kakaroto
Copy link
Contributor

This can lead to a crash if a timer was started right before deletion, so that when
the timer is launched, the self.parent() crashes.

Traceback (most recent call last):
  File "v:\Projects\book\manuskript-git\bin\..\manuskript\ui\editors\fullScreenEditor.py", line 314, in <lambda>
    self.timer.timeout.connect(lambda: self.parent().hideWidget(self))
RuntimeError: Wrapped C/C++ object of type myScrollbar has been deleted

I've had this error happen to me multiple times after pressing F11 once as soon as I start the app, but I've been unable to reproduce it after adding debug messages (and also after removing them), so it looks like a heisenbug, but this should fix the problem, even though I can't test that it works, it seems logical. I'm also unsure on why a scrollbar would get destroyed after I enter fullscreen mode once without exiting it.

@kakaroto
Copy link
Contributor Author

I just had the crash again with this fix, as well as with a self.timer = None added (I was hoping for a self.timer.start() Null pointer exception instead). So this doesn't fix it.
Still investigating but I can now reproduce the issue, and working on a fix. To reproduce, keep pressing F11 repeatedly while scrolling the mouse wheel, it will eventually crash after 3 or 4 times (note, this is on Windows).
I found the issue is that __del__ isn't called, probably due to the garbage collector not happening when the scrollbar object gets destroyed, so I think the solution now is to just do what I did on a 'destroyed' callback instead of __del__. I'm working on it now, then will test, and push an updated commit.

…mer signal

The lambda function will keep a reference to the scrollbar python object preventing it
from getting destroyed when the QScrollbar is destroyed. This causes the underlying
QT widget to be freed while the python object still exists, therefore the timer itself
doesn't get stopped/cleaned, so the timer will get called and cause a crash with :
"RuntimeError: Wrapped C/C++ object of type myScrollbar has been deleted"

To reproduce, press F11 repeatedly while scrolling.
@kakaroto
Copy link
Contributor Author

kakaroto commented Feb 21, 2019

I figured it out. Sorry for the first patch. You can reproduce by pressing F11 repeatedly fast enough, no need to scroll or anything. The issue was in using a lambda function for the timer. If you add prints in the __init__ and in a __del__, you'll see the myScrollBar doesn't get deleted if a lambda is used (just replace the connect(self.hide) with connect(lambda: self.hide()) to test).
I also used a sip.isdeleted(self) to confirm and test multiple times with a lambda but avoiding a crash.

Edit: I think the issue is that with the lambda, the timer itself keeps a reference on the python object, preventing it from getting deleted. When I was testing before, I had changed the code to use a function instead of lambda in order to add my print, which is why it became impossible to reproduce.
Also, note that having a self.destroyed.connect() doesn't seem to work, the destroyed callback is simply never called.

@kakaroto kakaroto changed the title On deletion, stop timer of myScrollBar from the fullscreen editor Fix fullscreen editor's myScrollBar delayed destruction causing a crash Feb 21, 2019
@gedakc
Copy link
Collaborator

gedakc commented Feb 25, 2019

Thank you @kakaroto for explaining how to reproduce the crash, and also providing a patch to fix the issue.

On Kubuntu 16.04 I was able to reproduce the crash by loading a project into manuskript and then pressing F11 in rapid succession.

I plan to review the Pull Requests in order of creation. If you would prefer a different order of review then let me know.

I will merge this PR.

Curtis

@gedakc gedakc merged commit 4a9937f into olivierkes:develop Feb 25, 2019
@gedakc gedakc added this to the 0.9.0 milestone Feb 25, 2019
@kakaroto kakaroto deleted the fix-fullscreen-crash branch February 25, 2019 19:06
@kakaroto
Copy link
Contributor Author

Hey @gedakc, I hope you enjoyed your long weekend!
Thanks for the quick review/merge. You can review the PRs in order of creation although it might be best to review bugfixes first and new features later as bugfixes should be easier/faster to review.
In either case, take your time, I know I've spammed the PR list over the weekend and that it might take you some time to review it all.
Thanks!

@gedakc
Copy link
Collaborator

gedakc commented Feb 25, 2019

Good suggestion. I will review the bug fixes first before moving on to the new features, such as alternate spell checkers. It may take me a few days as I have other project work too.

Ideally I would like to get your patches reviewed and merged with an eye towards creating a new Manuskript release in early March.

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

2 participants