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.

Sign up for GitHub

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

Jump to bottom

qgsdockablewidgethelper: Prevent crash when closing dock #59481

Merged
merged 1 commit into from
Nov 19, 2024
Merged

qgsdockablewidgethelper: Prevent crash when closing dock #59481

merged 1 commit into from
Nov 19, 2024

Conversation

ptitjano
Copy link
Contributor

Description

When closing the dock, QGIS sometimes crash. It seems like the mDialog::finished slot sends visibilityChanged signal. However, the QgsDockableWidgetHelper may have been destroyed in the meantime. Hence, the crash.

I tried to solve this issue by ensuring to emit this signal if the object still exists. I cannot reproduce the issue anymore but I'm not sure this is the proper fix.

@ptitjano ptitjano self-assigned this Nov 18, 2024
@github-actions github-actions bot added this to the 3.42.0 milestone Nov 18, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 18, 2024 •
edited
Loading

🪟 Windows builds

Download Windows builds of this PR for testing.
Debug symbols for this build are available here.
(Built from commit da73063)

🪟 Windows Qt6 builds

Download Windows Qt6 builds of this PR for testing.
(Built from commit da73063)

@nyalldawson
Copy link
Collaborator

I tried to solve this issue by ensuring to emit this signal if the object still exists. I cannot reproduce the issue anymore but I'm not sure this is the proper fix.

I don't think it's the correct fix either. The lambda is already referencing mDialog directly on line 319, so that means the only time that mDialog is nullptr on line 321 is if some connection to the closed signal emitted on line 320 has done something bad.

My suspicion would be that the issue is in one of the connections to that closed signal. Where specifically did you see this crash?

@ptitjano
qgisapp: Ensure to properly delete 2D MapCanvasDock
da73063 
When closing a 2D canvas dock, QGIS sometimes crashes. This is because
when the widget helper is closed, the canvas is closed. However, some
signals of the canvas widget may still be called after it is
called. Hence, the crash.

This issue is fixed by calling `deleteLater()` instead of `delete` to
ensure a proper deletion which properly takes into account the
signals.
@ptitjano
Copy link
Contributor Author

I tried to solve this issue by ensuring to emit this signal if the object still exists. I cannot reproduce the issue anymore but I'm not sure this is the proper fix.

I don't think it's the correct fix either. The lambda is already referencing mDialog directly on line 319, so that means the only time that mDialog is nullptr on line 321 is if some connection to the closed signal emitted on line 320 has done something bad.

My suspicion would be that the issue is in one of the connections to that closed signal. Where specifically did you see this crash?

Thanks. To trigger the crash:

  • Create a new 2D view
  • Click on the dock button
  • Close the view
    Sometimes, it crashes the first time, sometimes I need to do it 2 or 3 times but it always crashes at some point.

@ptitjano
Copy link
Contributor Author

This should be the proper fix now.

@nyalldawson
Copy link
Collaborator

LGTM! thanks for the extra investigation !

@nyalldawson nyalldawson merged commit 04446f9 into qgis:master Nov 19, 2024
37 checks passed
This was referenced Nov 19, 2024
Merged
Merged
@ptitjano ptitjano deleted the qgsdockablewidget-crash branch November 20, 2024 09:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nyalldawson nyalldawson nyalldawson approved these changes

Assignees

@ptitjano ptitjano

Labels
backport queued_ltr_backports Queued Backports backport release-3_40 Crash/Data Corruption
Projects
None yet
Milestone
3.42.0
Development

Successfully merging this pull request may close these issues.
2 participants
@ptitjano @nyalldawson