Use weakThisCopy as a getter instead of weakThis in cascadia/TerminalApp/Tab.cpp

[benediktjohannes]

Issue

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).

Changes I made

We now use the copy of weakThis , so weakThisCopy, instead in order to ensure correct runtime execution (but I've not tested it, it's just a code review, but I'm pretty confident that this is correct).

PR Checklist

• [ x] Closes Wrong weakThisGetter in TerminalApp/Tab.cpp at if (auto tab{ weakThis.get() }) where weakThisCopy.get() should be used instead #19855
• Tests added/passed (I've not tested this as described in the issue, it's just a code review, but I'm pretty confident that it's correct)
• Documentation updated
  • If checked, please file a pull request on our docs repo and link it here: #xxx
• Schema updated (if necessary)

[benediktjohannes]

Ah, I guess that extra line at the bottom lets the Code Health Check fail, I'll fix this in a moment

[benediktjohannes]

That's kind of weird, I removed it, but it wasn't actually removed, I'll try removing two in one step because originally I didn't add one, so maybe it's always automatically added

[benediktjohannes]

That actually worked 🤣

[DHowett]

Hmm. Since it doesn't extend the lifetime, we might as well drop weakThisCopy completely (unless we can find a compelling reason for it to exist) and just use the weakThis we stored in the capture list.

[benediktjohannes]

Sounds reasonable! So then in the other cases as well, right?

[benediktjohannes]

I‘m just wondering why this was added then if it‘s unnecessary?

[benediktjohannes]

Isn‘t this maybe due to changing processes inside the if Statement? Im sorry, I am Not Deep into the code, I just thought about this inconsistency.

[benediktjohannes]

I did some research on this and Here is what I found out: (thats probably the reason why it was added Originally)

events.TitleChanged = content.TitleChanged(
winrt::auto_revoke,
[dispatcher, weakThis](auto&&, auto&&) -> safe_void_coroutine {
// The lambda lives in the std::function-style container owned by control. That is, when the
// control gets destroyed the lambda struct also gets destroyed. In other words, we need to
// copy weakThis onto the stack, because that's the only thing that gets captured in coroutines.
// See: https://devblogs.microsoft.com/oldnewthing/20211103-00/?p=105870
const auto weakThisCopy = weakThis;
co_await wil::resume_foreground(dispatcher);
// Check if Tab's lifetime has expired
if (auto tab{ weakThisCopy.get() })
{
// The title of the control changed, but not necessarily the title of the tab.
// Set the tab's text to the active panes' text.
tab->UpdateTitle();
}
});

I‘ll have a deeper look on this, whether it still seems relevant or Not

[benediktjohannes]

I recommend reading the very interesting article about this linked above on the comment. This seems to be a very insidious thing and while I‘m Not quite sure whether this is fixed, I‘d recommend that we let the copy exist and use it. But I‘m Not quite sure about it, so please correct me if you have another opinion on this @DHowett. The only thing I‘m pretty sure about is that the current State with creating the unused variable is not the best solution and the inconsistency to the others as well, but Im fine with removing the copy if you think different about it and say that it is not needed any more. Thank you! So your opinion would be very interesting! 👍

[benediktjohannes]

So my very first explaination of why this seems to be needed to me is maybe not completely correct, but based on what I‘ve now found out it seems to me that the idea in the first place that this has any reason for existancy is probably correct and I‘ve now found something else that is very interesting: It seems to me that back then (when the comment still existed and probably already when the copys were added originally) this statement was already missed, so I think that using the copy here would be correct because (at least some if not all) use cases of the other copys were not removed either.

[benediktjohannes]

Addition: This seems to be still relevant because of the co_await if not fixed in general

[benediktjohannes]

But I am Not quite sure about this whole thing because I am new to Terminal, so I‘m very excited to hear about your opinion on this

[lhecker]

Thanks!

[lhecker]

Hmm. Since it doesn't extend the lifetime, we might as well drop weakThisCopy completely (unless we can find a compelling reason for it to exist) and just use the weakThis we stored in the capture list.

Lambda captures don't survive coroutine suspensions. That's pretty much it...