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

wxwidgets: fix assertion using hide in destroy #30909

Closed
wants to merge 2 commits into from

Conversation

ylwghst
Copy link
Contributor

@ylwghst ylwghst commented Oct 28, 2017

Motivation for this change

I have been experiencing errors while using FileZilla due to the bug in wxwidgets library.
Annoying pop up with error message is appearing every time FileZilla is started.

wxTopLevelWindowBase::Destroy calls Hide on itself if there is at least one other top level window.
Unfortunately calling Hide on a window that has not yet been created causes an assertion on wxGTK.
It is very easy to reproduce with this snippet you could for example add to the minimal sample in

MyFrame::OnAbout:
(new wxDialog)->Destroy();

Both WX_3_0_BRANCH and master are affected by this issue.

The error messages is here: https://gist.github.com/anonymous/38f27cbdc43c96a9c7fe063c3a10a08c

The bug is tracked here: https://trac.wxwidgets.org/ticket/17942

Patch source available here: https://trac.wxwidgets.org/attachment/ticket/17942/fix_assertion_using_hide_in_destroy.diff

I have attached a patch for this issue. With it, Hide is only called if the window is actually shown.

The attached patch has been created against WX_3_0_BRANCH but also applies cleanly against master.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • [+] NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • [+] Tested execution of all binary files (usually in ./result/bin/)
  • [+] Fits CONTRIBUTING.md.

@ylwghst ylwghst force-pushed the wxwidgets branch 4 times, most recently from 8bcc58c to d2aaefa Compare October 28, 2017 22:31
@@ -22,6 +22,10 @@ stdenv.mkDerivation rec {

nativeBuildInputs = [ pkgconfig ];

patches = [
./patches/fix_assertion_using_hide_in_destroy.diff
Copy link
Contributor

Choose a reason for hiding this comment

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

The patch should not be placed into the repository but fetched using fetchurl.

@disassembler
Copy link
Member

Looks good to me. @vcunat you want this rebased on staging with the 100-500 linux rebuilds it affects?

@vcunat
Copy link
Member

vcunat commented Nov 1, 2017

100–500 seems generally OK for master. Nitpick: the patches seem non-generated, so there's no real advantage of using fetchpatch instead of fetchurl, but it doesn't really matter.

@vcunat vcunat self-assigned this Nov 4, 2017
@vcunat
Copy link
Member

vcunat commented Nov 4, 2017

There were multiple issues, so I rewrote it. The URL was to a patch rendered as HTML, the patch won't apply to 2.8, and the hash didn't seem to match. I left 2.8 unpatched (it seems rarely used anyway).

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.

None yet

6 participants