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

Fix TrayIcon menu crash #16024

Merged
merged 3 commits into from
Jun 28, 2024
Merged

Fix TrayIcon menu crash #16024

merged 3 commits into from
Jun 28, 2024

Conversation

logiclrd
Copy link
Contributor

@logiclrd logiclrd commented Jun 13, 2024 •
edited
Loading

This PR addresses a crash that occurs when activating TrayIcon menu items. It contains two commits:

  • 4818fa9: Adds a new samples project called TrayIcon which should work. It follows all the guidelines, but with the current Avalonia code it crashes as soon as any tray icon menu item is activated.
  • bb3f939: Fixes the crash observed in the TrayIcon sample by thunking to the UI thread when processing a NativeMenuItem.CanExecuteChanged notification.

What is the current behavior?

At least on Ubuntu 24.04 running Xorg, activating a tray icon menu item causes the process to crash immediately with the exception:

Unhandled exception. System.InvalidOperationException: Call from invalid thread

This is described in detail in #15940.

What is the updated/expected behavior with this PR?

With the fix to NativeMenuItem.cs, tray icon menu items can now be activated without the process crashing.

How was the solution implemented (if it's not obvious)?

Dispatcher.UIThread.Invoke

Checklist

Fixed issues

Fixes #15940

@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.2.999-cibuild0049011-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@cla-avalonia
Copy link
Collaborator

cla-avalonia commented Jun 13, 2024 •
edited
Loading

  • All contributors have signed the CLA.

@logiclrd
Copy link
Contributor Author

@cla-avalonia agree

@logiclrd
Copy link
Contributor Author

(rebase)

@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.2.999-cibuild0049017-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

maxkatz6
maxkatz6 reviewed Jun 14, 2024
View reviewed changes
samples/TrayIcon/AboutWindow.axaml Outdated
@@ -0,0 +1,18 @@
<Window xmlns="https://github.com/avaloniaui"
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure about adding yet another project here.
Ideally, TrayIcon test should be added to the IntegrationTestApp project + new e2e tests written per each platform (can be done separately).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, okay. Coming at this from the other direction, when I first started writing my application, I found all existing documentation of TrayIcon to be incomplete and there were no samples. This would be the first sample showing how to use TrayIcon as far as I am aware. It serves to illustrate the crash in this PR, but after the PR is merged, it serves as a TrayIcon example for new developers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a coded UI test infrastructure in place that would allow automated testing of this? If so then it's also a good point to add a test of this crash to that as well. :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I duplicated the TrayIcon stuff into IntegrationTestApp but no menu appears when I click on the tray icon. I tried a bunch of different ways to make IntegrationTestApp be more like TrayIcon but I wasn't able to figure out what the difference is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see project Avalonia.IntegrationTests.Appium. Does Appium support interacting with tray icons? I tried some searches related to this and couldn't find anything conclusive. I've never worked with Appium before.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, well, I tried to merge it in there, but when I run the IntegrationTestApp project, the menu doesn't show up when clicking on the tray icon. Not sure how it's different from the TrayIcon sample, but there must be something...

    <TrayIcon.Icons>
        <TrayIcons>
            <TrayIcon Icon="/Assets/icon.ico">
                <TrayIcon.Menu>
                    <NativeMenu>
                        <NativeMenuItem Header="Show _Test Window" Command="{Binding ShowWindowCommand}" />
                    </NativeMenu>
                </TrayIcon.Menu>
            </TrayIcon>
        </TrayIcons>
    </TrayIcon.Icons>

Copy link
Member

Choose a reason for hiding this comment

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

Works here. Right click. But I removed command as it doesn't have view model locally.

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's very strange. It continues to not work for me. I tried removing the command binding just in case that was why but it didn't fix it. Since I can't get the menu to show up, I can't confirm the crash with IntegrationTestApp. It's a technicality but I would feel better if you could confirm the crash without c71d999 and the fix with it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or, rather, with and without 04e3225. :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay I switched from GNOME Desktop to KDE Plasma and now the menu shows up, and I can confirm the fix. :-) Definitely crashes without 04e3225, definitely fixed with, at least on my end.

@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.2.999-cibuild0049160-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.2.999-cibuild0049172-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@logiclrd logiclrd force-pushed the JDG_TrayIconMenuCrash branch 2 times, most recently from 6430a46 to 4ce9ad7 Compare June 20, 2024 14:28
@logiclrd
Copy link
Contributor Author

(Restored the command binding, because it does in fact work. :-) )

@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.2.999-cibuild0049176-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@logiclrd logiclrd mentioned this pull request Jun 21, 2024
Closed
@logiclrd
Updated the TrayIcon in IntegrationTestApp to have an associated menu.
54fdd0f 
Updated App.axaml.cs to set the DataContext for binding and provide a command implementation.
Added a project reference to Avalonia.ReactiveUI.
@logiclrd
Updated CanExecuteChanged in NativeMenuItem.cs to thunk to the UI thr…
5afd3b9 
…ead if it ends up running on a non-UI thread.
@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.2.999-cibuild0049526-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@maxkatz6 maxkatz6 added the bug label Jun 28, 2024
@maxkatz6 maxkatz6 merged commit 79f29cc into AvaloniaUI:master Jun 28, 2024
3 of 10 checks passed
@maxkatz6 maxkatz6 mentioned this pull request Jun 28, 2024
Open
@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.2.999-cibuild0049544-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

maxkatz6 added a commit that referenced this pull request Jun 28, 2024
@logiclrd @maxkatz6
Fix TrayIcon menu crash (#16024)
a0286f3 
* Updated the TrayIcon in IntegrationTestApp to have an associated menu.
Updated App.axaml.cs to set the DataContext for binding and provide a command implementation.
Added a project reference to Avalonia.ReactiveUI.

* Updated CanExecuteChanged in NativeMenuItem.cs to thunk to the UI thread if it ends up running on a non-UI thread.

* Replace ReactiveUI with MiniMvvm to simplify integration tests app

---------

Co-authored-by: Max Katz <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maxkatz6 maxkatz6 maxkatz6 approved these changes

Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

"Call from invalid thread" process crash after tray icon menu item is invoked
4 participants
@logiclrd @avaloniaui-bot @cla-avalonia @maxkatz6