Fix crash after pressing Apply and Ok inside Settings widget. JB#49707

[sergey-levin]

I believe adding QEventLoop::ExcludeUserInputEvents in these four places is sufficient.

[martyone]

100ms may be enough under some conditions but not generally reliable. I think it should be ensured already in the settings dialog implementation that it does not re-enter these code paths. It already does something to avoid similar issues with the OK button hit repeatedly, so maybe something like:

diff --git a/src/plugins/coreplugin/dialogs/settingsdialog.cpp b/src/plugins/coreplugin/dialogs/settingsdialog.cpp
index 596d498875..7d77828cbd 100644
--- a/src/plugins/coreplugin/dialogs/settingsdialog.cpp
+++ b/src/plugins/coreplugin/dialogs/settingsdialog.cpp
@@ -433,6 +433,7 @@ private:
     QLabel *m_headerLabel;
     std::vector<QEventLoop *> m_eventLoops;
     bool m_running = false;
+    bool m_applying = false;
     bool m_applied = false;
     bool m_finished = false;
 };
@@ -691,7 +692,7 @@ void SettingsDialog::filter(const QString &text)
 
 void SettingsDialog::accept()
 {
-    if (m_finished)
+    if (m_applying || m_finished)
         return;
     m_finished = true;
     disconnectTabWidgets();
@@ -705,7 +706,7 @@ void SettingsDialog::accept()
 
 void SettingsDialog::reject()
 {
-    if (m_finished)
+    if (m_applying || m_finished)
         return;
     m_finished = true;
     disconnectTabWidgets();
@@ -716,8 +717,12 @@ void SettingsDialog::reject()
 
 void SettingsDialog::apply()
 {
+    if (m_applying)
+        return;
+    m_applying = true;
     foreach (IOptionsPage *page, m_visitedPages)
         page->apply();
+    m_applying = false;
     m_applied = true;
 }
 

[sergey-levin]

100ms may be enough under some conditions but not generally reliable.

Did you notice, that QTimer restarting every 100ms? So widget will be deleted eventually.

---

so maybe something like

Your suggestion has not much difference from this PR: #295
With addition
if (m_storing) return;
at the beginning of void MerBuildEngineOptionsWidget::store() it should do the same.

---

I found bigger problem:
instructions flow may return from this line

sailfish-qtcreator/src/libs/sfdk/vmconnection.cpp

Line 275 in 6de6df1

loop.exec();

to this

sailfish-qtcreator/src/plugins/mer/merbuildengineoptionswidget.cpp

Line 164 in 6de6df1

for (BuildEngine *const buildEngine : qAsConst(m_buildEngines)) {

and at line 164 widget MerBuildEngineOptionsWidget could be already deleted by

sailfish-qtcreator/src/plugins/mer/merbuildengineoptionspage.cpp

Line 79 in 6de6df1

delete m_widget;

This will cause the crash. And this is the reason why m_widget being deleted so strange way.

[martyone]

Yeah I didn't notice it will try repeatedly, but that doesn't really make it different - the dialog should simply not close until the saving finishes. Similarly, extending #295 with if (m_storing) return would not fix that.

[martyone]

I forgot to comment on your other findings (under line): isn't that also adrressed by the suggested change on SettingsDialog side?

[sergey-levin]

isn't that also adrressed by the suggested change on SettingsDialog side?

I tested your solution - it is fix the issue. Just one note: pressing Apply and Ok, makes Ok button unresponsive until settings are stored. It is a little issue, but I would suggest setOverrideCursor(Qt::WaitCursor)/restoreOverrideCursor around

sailfish-qtcreator/src/plugins/mer/merbuildengineoptionspage.cpp

Line 71 in 94ad623

m_widget->store();

to notify user.

[martyone]

Yup, or better buttonBox->disable/enable inside SettingsDialog::apply().

[sergey-levin]

So, this PR should be closed, right?

[martyone]

I would rather continue under this PR instead of opening a new one. Just rework the commit (I would consider to do it with two commits, separating the buttonBox disabling), submit it for upstream review, and once upstream accepts it, we will merge this PR.

[sergey-levin]

Ok, will do.

[sergey-levin]

PR: https://codereview.qt-project.org/c/qt-creator/qt-creator/+/298277
martyone, would you comment there about

OK button hit repeatedly

issue?

[martyone]

@sergey-levin I think your inputs there are well put and sufficient. I will wait how they react now.

[sergey-levin]

I will wait how they react now.

Well, I failed to convince a reviewer.

Personally, I think he is right at the point that QEventLoop::exec is dangerous. It is used here

sailfish-qtcreator/src/libs/sfdk/vmconnection.cpp

Line 1196 in 6854350

loop->exec();

and cause the crash. Alas, I see no way how to remove it without too much effort.

[martyone]

@sergey-levin I agree, we should fix it different way - there is already a related FIXME

sailfish-qtcreator/src/libs/sfdk/virtualmachine.h

Line 144 in 6854350

// FIXME API style?

but I do not have a full answer at hand. I will discuss it with the team and ping you later.

[martyone]

To me it seems it now leads to re-entering widget's store() when OK is pressed shortly after Accept and it's just by chance that it has no visible bad effect. Accept -> store() -> QEventLoop -> Ok -> store().

I am afraid fixing this properly would likely require significant changes to the UI. All properties that cannot be changed instantly (and also those that we can fail to change) would need dialog based UIs to change them immediately (no Cancel).

Thinking about other possible workarounds, how about trying to decrease the minimum duration of the progress dialog to 700ms? Wouldn't that effectively prevent user hitting the other button under normal circumstances?

[sergey-levin]

To me it seems it now leads to re-entering widget's store() when OK is pressed shortly after Accept and it's just by chance that it has no visible bad effect. Accept -> store() -> QEventLoop -> Ok -> store().

Yes, you are right. We may use m_storing variable, to prevent storing twice.

I am afraid fixing this properly would likely require significant changes to the UI. All properties that cannot be changed instantly (and also those that we can fail to change) would need dialog based UIs to change them immediately (no Cancel).

So true. Personally, by my goodwill, I would not start such task - too many things could be broken.

Thinking about other possible workarounds, how about trying to decrease the minimum duration of the progress dialog to 700ms? Wouldn't that effectively prevent user hitting the other button under normal circumstances?

I think it will not help. I am able to crash QtC by pressing Apply, then hit Esc button.

[martyone]

I think it will not help. I am able to crash QtC by pressing Apply, then hit Esc button.

Right.

It seems the UI change is inevitable, but maybe we could try to temporarily work it around with QEventLoop::ExcludeUserInputEvents as Eike suggested. It wouldn't freeze UI in the sense that it does not repaint. It would just not handle user input, but that doesn't really change anything as we do not currently allow the user to cancel these long running operations, right?

[sergey-levin]

It seems the UI change is inevitable, but maybe we could try to temporarily work it around with QEventLoop::ExcludeUserInputEvents as Eike suggested.

It might help, in case we change in two places:

• sailfish-qtcreator/src/libs/sfdk/asynchronous.h

  Line 54 in d543afa

  loop.exec();

• sailfish-qtcreator/src/libs/sfdk/vmconnection.cpp

  Line 1206 in d543afa

  loop->exec();

It is only what I found, it maybe more. As for me - it might cause too big impact on app behaviour. Deleting finish() procedure have lesser impact I believe.

[martyone]

There are more QEventLoop::exec call there. At least the one in VmConnection::lockDown is involved.

Do not forget deleting finish() would lead to re-entering store() and who know what else, which is not nice.

[sergey-levin]

deleting finish() would lead to re-entering store()

Check if (m_storing) return; before line

sailfish-qtcreator/src/plugins/mer/merbuildengineoptionswidget.cpp

Line 154 in d543afa

m_storing = true;

will prevent re-entering.

So, you are really propose to add QEventLoop::ExcludeUserInputEvents to all involved event loops? I tested it a little - it seems ok. But if operation freeze - entire UI will freeze and user will not able to do anything with it.

[martyone]

Yeah I forgot that we already have m_storing there.

If an operation freezes (like forever) then we are in troubles anyway - there is no way to cancel. And if we accidentally let the user hit escape key and escape the option dialog while a frozen operation was active, no other operation would be ever allowed to start (asynchronous operations are strictly serialized). So sooner or later the UI would freeze anyway.

We do not run operations that are likely to freeze, so it would be wasted power trying to deal with such unlikely case. If canceling is enabled, then the part of the work that was already done must be undone and this would bring it to a completely new level when comes to complexity while most of it would be dead code 99% of time. So better be patient than bringing the installation in some inconsistent state that would be hard to fix.

So yes, trying with QEventLoop::ExcludeUserInputEvents looks like the least bad option.