Skip to content

Wrong weakThisGetter in TerminalApp/Tab.cpp at if (auto tab{ weakThis.get() }) where weakThisCopy.get() should be used instead #19855

@benediktjohannes

Description

@benediktjohannes

In terminal/src/cascadia/TerminalApp/Tab.cpp there is always created a copy of weakThis called weakThisCopy

(e.g. in events.TabColorChanged = content.TabColorChanged( winrt::auto_revoke, [dispatcher, weakThis](auto&&, auto&&) -> safe_void_coroutine { const auto weakThisCopy = weakThis; co_await wil::resume_foreground(dispatcher); if (auto tab{ weakThisCopy.get() }) { // The control's tabColor changed, but it is not necessarily the // active control in this tab. We'll just recalculate the // current color anyways. tab->_RecalculateAndApplyTabColor(); tab->_tabStatus.TabColorIndicator(tab->GetTabColor().value_or(Windows::UI::Colors::Transparent())); } }); or in events.ConnectionStateChanged = content.ConnectionStateChanged( winrt::auto_revoke, [dispatcher, weakThis](auto&&, auto&&) -> safe_void_coroutine { const auto weakThisCopy = weakThis; co_await wil::resume_foreground(dispatcher); if (auto tab{ weakThisCopy.get() }) { tab->_UpdateConnectionClosedState(); } });

)

while this copy is not used in one case

events.ReadOnlyChanged = content.ReadOnlyChanged( winrt::auto_revoke, [dispatcher, weakThis](auto&&, auto&&) -> safe_void_coroutine { const auto weakThisCopy = weakThis; co_await wil::resume_foreground(dispatcher); if (auto tab{ weakThis.get() }) { tab->_RecalculateAndApplyReadOnly(); } });

where only weakThis is used and in my opinion (it was not tested, it's just a code review) the copy of weakThis, so weakThisCopy, should be used because it's done so in the other cases as well and it makes sense to me because of potential changes during the execution of the code of weakThis itself. And I don't see any difference in this case to the other cases - while only it refers to something different (ReadOnlyChanged'), but it still includes the creation of the const auto weakThisCopywhich indicates to me that this should also be used (because otherwise the creation of the variable would not make any sense). And allover I'm pretty sure that there is no difference inReadOnlyChanged` to the other cases, so I'd recommend using the copy instead (but I've not tested it, it's just a code review, but I'm pretty confident that this is correct).

Metadata

Metadata

Assignees

No one assigned

    Labels

    Needs-Tag-FixDoesn't match tag requirementsNeeds-TriageIt's a new issue that the core contributor team needs to triage at the next triage meeting

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions