Skip to content
This repository has been archived by the owner on Feb 12, 2023. It is now read-only.

fix(widget): update local Widget Core pointer when changing core #5766

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

Conversation

Kribylet
Copy link
Contributor

@Kribylet Kribylet commented Aug 5, 2019

Widget carried a Core* that wasn't being updated when changing Core, this commit fixes that. Widget::addFriend would add duplicate friends when restarting a Core, so a check was added to prevent adding duplicates of friends.

Fixes #5758


This change is Reviewable

Copy link
Member

@sudden6 sudden6 left a comment

Choose a reason for hiding this comment

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

:lgtm_strong:

Reviewed 1 of 1 files at r1.
Reviewable status: :shipit: complete! 1 of 1 LGTMs obtained

@hart0
Copy link

hart0 commented Aug 5, 2019

I tried these changes and it fixes the graphical issues showing duplicated friends and stops the crashing when reconnecting, however attempting to send any message to a friend after the reconnect is done and my status is back online causes a crash.

Thread 1 "qtox" received signal SIGSEGV, Segmentation fault.
0x00000000006d327c in std::__invoke_impl<bool, bool (ICoreFriendMessageSender::* const&)(unsigned int, QString const&, NamedType<unsigned int, ReceiptNumTag, Orderable>&), ICoreFriendMessageSender&, unsigned int&, QString const&, NamedType<unsigned int, ReceiptNumTag, Orderable>&> (__f=
    @0x7fffffffcf60: &virtual table offset 24, __t=...,
    __args#0=@0x7fffffffcf5c: 0, __args#1=..., __args#2=...)
    at /usr/include/c++/9/bits/invoke.h:66
66      { return (__invfwd<_Tp>(__t).*__f)(std::forward<_Args>(__args)...); }

bt

@sudden6
Copy link
Member

sudden6 commented Aug 10, 2019

@Kribylet can this be closed now that we agreed on #5778?

@Kribylet
Copy link
Contributor Author

@Kribylet can this be closed now that we agreed on #5778?
I believe so, since there would be no way to change the core of an application instance in that case.

@anthonybilinski
Copy link
Member

Now that v1.17-dev is branched from master and #5778 has been merged to it, I think to continue to work towards (working) reconnect functionality in the future, backing out #5778 on master and continuing this work on top would be reasonable, targeting master branch with the PR.

v1.17-dev will have no reconnect button, but master branch and future releases can have a working reconnect button when this is finished.

@sudden6
Copy link
Member

sudden6 commented Aug 29, 2019

@anthonybilinski I'm against re-introducing the reconnect button for now, it's still not very useful.

@Kribylet Kribylet force-pushed the fix_reconnect_segfault branch from bcde006 to 0cbb379 Compare August 29, 2019 07:27
@Kribylet
Copy link
Contributor Author

Pushed the latest version of this PR from my local repository for potential use later, if we want to restore reconnect button functionality. This commit sequence changes the tox instance used by the Core instance, rather than replace the Core instance entirely.

I advocate harsh testing before applying this PR, but naively I wouldn't expect any issues with this that couldn't already occur from having a bad connection.

Personally tested using qtox normally, then reconnecting, resuming chat, and using AV features post reconnect.

@Kribylet Kribylet force-pushed the fix_reconnect_segfault branch from 0cbb379 to b8cd25b Compare August 29, 2019 07:51
@anthonybilinski
Copy link
Member

@sudden6 even if it's fully working in a future release? I thought removing it was a temporary measure?

@jessica181920
Copy link
Contributor

@sudden6 Clicking a button is much easier than closing the client and signing back in manually.

jayzhan211 pushed a commit to jayzhan211/qTox that referenced this pull request Jan 14, 2020
anthonybilinski pushed a commit to anthonybilinski/qTox that referenced this pull request Jan 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clicking ‘reconnect’ under Connection Settings causes Segmentation fault
5 participants