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

libsForQt5.qmltermwidget: Fetch patch to Lomiri's patch series #275475

Merged

Conversation

OPNA2608
Copy link
Contributor

@OPNA2608 OPNA2608 commented Dec 19, 2023

...to fix a crash in lomiri-terminal-app.

Description of changes

Working towards #99090.

A debugging of this issue can be found here: https://gitlab.com/ubports/development/apps/lomiri-terminal-app/-/issues/104#note_1662252236
I have submitted this to gber/qmltermwidget#1, where it (or a variant of it) should hopefully find its way into the UBports/Lomiri patch series.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
    Current hardware doesn't have enough RAM for nixpkgs-review. I tested cool-retro-term (unaffected by this, but doesn't hurt I guess) and lomiri.lomiri-terminal-app
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@OPNA2608 OPNA2608 force-pushed the fix/qmltermwidget-ColorSchemeManager-instance branch from ee5ceb4 to 3a96799 Compare January 14, 2024 22:10
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

so looking at the patch, other than i am not familiar with the codebase, is that it sets the owner on every call -- seems ideally that would happen just once when the instance is created.

i suppose that this is a read / write object and if there is concurrent access that is gated elsewhere, but if it is supposed to be just read-only (and not returned as const)

QQmlEngine::setObjectOwnership(instance, QQmlEngine::CppOwnership);

as long as that is just writing the same data to the same location without side effects, that seems fine to me.

@OPNA2608
Copy link
Contributor Author

it sets the owner on every call -- seems ideally that would happen just once when the instance is created.

AFAIU, it does only set it once. colorschememanager_provider is used as a callback for qmlRegisterSingletonType to setup a singleton instance of a type for the QML side to use. The expectation is normally that this callback returns a unique object, which will then be managed by the QML engine & free'd on shutdown. But since this re-uses a global object automagically created by Qt on startup it must be marked as C++-owned so the QML engine doesn't try to free an object that it doesn't own.

I'm not sure how concurrent R/W access is gated, or if that's even necessary, and my patch doesn't really affect that AFAICT. It's just about marking object lifetime & responsibilities, which are currently wrong & cause a SIGABRT with free(): invalid pointer on shutdown.

@ghost
Copy link

ghost commented Jan 14, 2024

it sets the owner on every call -- seems ideally that would happen just once when the instance is created.

AFAIU, it does only set it once.

that works for me -- preventing SIGABRT is a definite improvement.

...to fix a crash in lomiri-terminal-app.
@OPNA2608 OPNA2608 force-pushed the fix/qmltermwidget-ColorSchemeManager-instance branch from 3a96799 to 84127ed Compare January 14, 2024 23:07
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

LGTM

@ghost ghost merged commit 2f076b5 into NixOS:master Jan 15, 2024
22 of 23 checks passed
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant