Re-enable web-source icons in Stable and Preview builds

[DHowett]

Disables a controversial part of #19044.

Refs #19075

[e82eric]

@DHowett this might be something I am doing wrong but I started seeing a crash for the commandHistory suggestion source after merging this.

To repro: it usually triggered when I searched for something and then ctrl+backspace to clear the search. (Sometimes I had to do that a couple times). I couldn't repro it after commenting out this line

terminal/src/cascadia/TerminalSettingsModel/Command.cpp

Line 737 in a04e410

command->_iconPath = iconPath;

CrecentCommands\src\terminal-suggestions\src\cascadia\TerminalApp\Generated Files\winrt\Windows.UI.Xaml.Controls.h(4285)\TerminalApp.dll!00007FFD23AD7E42: LogHr(1) tid(bf94) 80070057 The parameter is incorrect.
    [auto __cdecl winrt::impl::consume_Windows_UI_Xaml_Controls_IContentPresenter<struct winrt::Windows::UI::Xaml::Controls::IContentPresenter>::Content(const struct winrt::Windows::Foundation::IInspectable &) const]
Exception thrown at 0x00007FFE3BB800AC (KernelBase.dll) in WindowsTerminal.exe: WinRT originate error - 0x80070057 : 'The parameter is incorrect.'.
\src\terminal-suggestions\src\cascadia\TerminalApp\Generated Files\winrt\Windows.UI.Xaml.Controls.h(4285)\TerminalApp.dll!00007FFD235A3406: LogHr(2) tid(bf94) 80070057 The parameter is incorrect.
    [auto __cdecl winrt::impl::consume_Windows_UI_Xaml_Controls_IContentPresenter<struct winrt::Windows::UI::Xaml::Controls::IContentPresenter>::Content(const struct winrt::Windows::Foundation::IInspectable &) const]
Exception thrown at 0x00007FFE3BB800AC in WindowsTerminal.exe: Microsoft C++ exception: winrt::hresult_invalid_argument at memory location 0x000000F162D1B990.
Exception thrown at 0x00007FFE3BB800AC in WindowsTerminal.exe: Microsoft C++ exception: [rethrow] at memory location 0x0000000000000000.
TerminalApp.dll!00007FFD23FD819C: ReturnHr(1) tid(bf94) 80070057 The parameter is incorrect.
    Msg:[winrt::hresult_error: The parameter is incorrect.] 
\src\terminal-suggestions\src\cascadia\TerminalApp\Generated Files\winrt\Windows.UI.Xaml.Controls.h(9381)\TerminalApp.dll!00007FFD236EBC82: LogHr(3) tid(bf94) 80070057 The parameter is incorrect.
    [auto __cdecl winrt::impl::consume_Windows_UI_Xaml_Controls_IListViewBase<struct winrt::Windows::UI::Xaml::Controls::ListView>::ScrollIntoView(const struct winrt::Windows::Foundation::IInspectable &) const]
Exception thrown at 0x00007FFE3BB800AC in WindowsTerminal.exe: Microsoft C++ exception: winrt::hresult_invalid_argument at memory location 0x000000F162D1C710.
Exception thrown at 0x00007FFE3BB800AC in WindowsTerminal.exe: Microsoft C++ exception: [rethrow] at memory location 0x0000000000000000.
TerminalApp.dll!00007FFD23F6F55C: ReturnHr(2) tid(bf94) 80070057 The parameter is incorrect.
    Msg:[winrt::hresult_error: The parameter is incorrect.] 
\src\terminal-suggestions\src\cascadia\TerminalApp\Generated Files\winrt\Windows.UI.Xaml.Controls.Primitives.h(3596)\TerminalApp.dll!00007FFD236EC5BA: LogHr(4) tid(bf94) 80070057 The parameter is incorrect.
    [auto __cdecl winrt::impl::consume_Windows_UI_Xaml_Controls_Primitives_ISelector<struct winrt::Windows::UI::Xaml::Controls::ListView>::SelectedIndex(int) const]
Exception thrown at 0x00007FFE3BB800AC in WindowsTerminal.exe: Microsoft C++ exception: winrt::hresult_invalid_argument at memory location 0x000000F162D1D630.
Exception thrown at 0x00007FFE3BB800AC in WindowsTerminal.exe: Microsoft C++ exception: [rethrow] at memory location 0x0000000000000000.
TerminalApp.dll!00007FFD23FD69CC: ReturnHr(3) tid(bf94) 80070057 The parameter is incorrect.
    Msg:[winrt::hresult_error: The parameter is incorrect.] 
Unhandled exception at 0x00007FFE1F4E5ED0 (Windows.UI.Xaml.dll) in WindowsTerminal.exe: 0xC000027B: An application-internal exception has occurred (parameters: 0x0000025FBE7DB300, 0x0000000000000004).

[DHowett]

Strange! I would expect the earlier PR about PaletteItem composition/inheritance to be the culprit. I can't pull up the number now from my phone however. :)

[DHowett]

@e82eric if you enable Exceptions>Win32>WinRT Originate, you may find the first failure that leads to these log messages and the eventual catch.

(If you enable it for process start-up you will also have to Continue through a few of them, because regrettably the Foundation.Collections module uses exception handling for flow control (TryLookup is just a try/catch around Lookup :p))

[e82eric]

@e82eric if you enable Exceptions>Win32>WinRT Originate, you may find the first failure that leads to these log messages and the eventual catch.

ahh, that is so good (this would have saved me so much time). Now I can see the exact stacktrace and that the IPaletteItem is null. (I am still puzzled why it is null)

[e82eric]

Strange! I would expect the earlier PR about PaletteItem composition/inheritance to be the culprit. I can't pull up the number now from my phone however. :)

I have a feeling you are right that it is the PaletteItem one, I didn't property bisect the commits and jumped to the conclusion it was the icon one from looking at the commit messages.

[e82eric]

I am in way over my head as far as how xaml templates vs controls work (and c++ in general). But I "think" what is happening is the _resolvedIcon field was getting reused across instances and getting in a invalid state.

I don't see the crash after removing the !_resolvedIcon (but doing that probably causes the icon to be resolved way more times than is intended).
https://github.com/microsoft/terminal/blob/main/src/cascadia/TerminalApp/BasePaletteItem.h#L17

[DHowett]

Hah, that was a last-minute throw-in too.

I'll have a look tomorrow. Thank you for tracking it down :)

[DHowett]

The weird thing is, if it were null it should fail immediately after when it calls Icon (on this as T*). I suppose that only matters if Icon is implemented? If Icon is not implemented, the null pointer passes through harmlessly. But there's still something calling methods on a null PaletteItem 😱

[e82eric]

I think I was wrong about the PaletteItem being null, it seemed more like is was in a bad state or corrupt.

[DHowett]

@e82eric I can't reproduce this at the tip of your PR branch unfortunately! Even on Windows 10!

[e82eric]

just to double check were you testing with the scrollBack source or the commandHistory source. I had commented out the icon on the scrollBack source //command->IconPath(L"\uE756"); so it should work but the commandHistory one should still fail.

[DHowett]

I tried to set up a scrollBack suggestion source. Let me try with command history. :)

[e82eric]

ahh that makes sense, if you uncomment this line it should repro with the scrollBack source also.

https://github.com/e82eric/terminal/blob/scrollback_suggestions_squash/src/cascadia/TerminalSettingsModel/Command.cpp#L753

[DHowett]

Uh, the XAML bindings are attempting to set the ContentPresenter's content to the ContentPresenter.

[DHowett]

Okay, this is something we didn't really account for (but should have.) IconElements are UI elements. They can't be cached and moved between UI surfaces willy-nilly.

The issue occurs when something with an icon changes physical position in the list due to filtering, and the IconElement has to get reparented to a new list item.

[DHowett]

great catch