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 crash in NTP servers GUI dialog on late network configuration #3252

Merged
merged 1 commit into from Mar 22, 2021

Conversation

rvykydal
Copy link
Contributor

Resolves: rhbz#1938168

@rvykydal
Copy link
Contributor Author

rvykydal commented Mar 19, 2021

The crash happens when trying to update an iterator from the Gtk model for list of NTP servers after the iterator becomes invalid (by clearing the model):

  • User enters DateTime Spoke -> on spoke refresh the model refresh (of ntp server state) is started: for iterator ITR_X from the model:
  • THREAD_Y starts to check server pointed by ITR_X with ntp_server_workig() method
  • User enters NTP configuration dialog -> model is reinitalized / cleared (servers/items are added from scratch)
  • THREAD_Y finishes the ntp_server_working() check and tries to update the ITR_X which no longer exists => crash

The issue is hit when the ntp_server_working takes too long (5 secs), ie longer than the time between entering the spoke and opening the NTP configuration dialog. For example when networking is configured later in the installation (like just before NTP configuration).

The crash showed with this
rvykydal@212eb1c
RVDBG debug info added:

anaconda.crash.log

If the ntp_server_working function is fast, eg network is activated early the refresh on spoke entry is fast enough to prevent the crash:

anaconda.ok.log

Seems to be related also to this change #2468
In upstream the code was completely rewritten in commit 15d2b2f

I think the safe fix for rhel8-branch would be checking the validity of iterator before using it as in this PR. In case of invalid itr there is no action so I think even the result should be correct.

We could probably also remove the model refresh (

gtk_call_once(self._config_dialog.refresh_servers_state)
) from spoke refresh method because the refreshed values are used/available (I think) only when the dialog is entered, and entering the dialog does reinitialization of the model anyway. Perhaps the refresh became useless (malign) after the #2468. I haven't found any reason for its existence but we should think twice here.

Copy link
Contributor

@VladimirSlavik VladimirSlavik left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thank you! And sorry for breaking it.

I'm actually amazed that we keep an iterator in the asynchronous call instead of just the server hostname. But I guess it's understandable how this came to be.

Copy link
Contributor

@poncovka poncovka left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks!

Copy link
Member

@jkonecny12 jkonecny12 left a comment

Choose a reason for hiding this comment

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

LGTM! Great work!

@rvykydal rvykydal merged commit 8165c08 into rhinstaller:rhel-8 Mar 22, 2021
@JeongHyeon-Kim
Copy link

Thank for solving issue and record

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants