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

ConnectDialog: re-arrange lookedUp() code to avoid recursive runloop problem. #3198

Merged
merged 1 commit into from Jul 27, 2017

Conversation

@mkrautz
Copy link
Member

commented Jul 23, 2017

This ensures that we apply all globally visible state from the lookedUp()
method before we attempt to auto-connect.

The previous block of code would, when auto-connect is enabled, call
accept() to perform the auto-connect. In case the selected ServerItem
has no username set, accept() will trigger a dialog box to appear asking
the user to input their desired username. Unfortunately, showing this
dialog causes Mumble to enter the event loop before the lookedUp()
method anticipated it to be entered. The point at which the auto-connect
happens, is before the lookedUp() method has had time to update the
global state of the ConnectDialog to reflect the successful host lookup.
In practice, this meant that the auto-connect would be triggered many,
many times in a row. In this scenario, Mumble would open a new username
dialog box for each auto-connect attempt. This would cause Mumble to
become unresponsive, because Mumble would spawn infinitely many username
dialog boxes.

The code of the lookedUp() method was re-arranged in
4d6e28e, which introduced this problem.

Fixes #3191

ConnectDialog: re-arrange lookedUp() code to avoid recursive runloop …
…problem.

This ensures that we apply all globally visible state from the lookedUp()
method before we attempt to auto-connect.

The previous block of code would, when auto-connect is enabled, call
accept() to perform the auto-connect. In case the selected ServerItem
has no username set, accept() will trigger a dialog box to appear asking
the user to input their desired username. Unfortunately, showing this
dialog causes Mumble to enter the event loop before the lookedUp()
method anticipated it to be entered. The point at which the auto-connect
happens, is before the lookedUp() method has had time to update the
global state of the ConnectDialog to reflect the successful host lookup.
In practice, this meant that the auto-connect would be triggered many,
many times in a row. In this scenario, Mumble would open a new username
dialog box for each auto-connect attempt. This would cause Mumble to
become unresponsive, because Mumble would spawn infinitely many username
dialog boxes.

The code of the lookedUp() method was re-arranged in
4d6e28e, which introduced this problem.

Fixes #3191
@hacst
hacst approved these changes Jul 27, 2017
Copy link
Member

left a comment

LGTM. I assume an alternative design would've been to do a QMetaObject::invokeMethod(this, "accept", Qt::QueuedConnection) instead of the direct call or is that racy?

@mkrautz mkrautz merged commit 4566f09 into mumble-voip:master Jul 27, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Enigma0

This comment has been minimized.

Copy link

commented Jul 28, 2017

@mkrautz What snapshot version might this fix be in?

@mkrautz

This comment has been minimized.

Copy link
Member Author

commented Jul 28, 2017

I've just triggered a new snapshot. Should propagate within the hour.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.