Lock for writing in ControlCore::SetBackgroundOpacity

[miniksa]

PR Checklist

• Closes random crash that @lhecker sent me on Teams
• I work here.

Detailed Description of the Pull Request / Additional comments

• Any change to the renderer engine has to be done under lock. Leonard gave me a crash where the dirty rectangles changed out from under the renderer thread. By inspection, only one spot in ControlCore is modifying the engine outside of lock.... here. The dump is too far along to definitively prove the issue and it's sort of a race so its difficult to repro. But the theory is sound that all writes to the dirty regions must be done under lock. So here's a fix.

[lhecker]

With a bit of regexp:

(render)?\S*engine\S*->(SetCallback|SetWarningCallback|ToggleShaderEffects|SetRetroTerminalEffect|SetPixelShaderPath|SetForceFullRepaintRendering|SetSoftwareRendering|GetSwapChainHandle|Invalidate|InvalidateCursor|InvalidateSystem|InvalidateSelection|InvalidateScroll|InvalidateAll|InvalidateCircling|PrepareForTeardown|StartPaint|EndPaint|RequiresContinuousRedraw|WaitUntilCanRender|Present|ScrollFrame|PrepareRenderInfo|PaintBackground|PaintBufferLine|PaintBufferGridLines|PaintSelection|PaintCursor|UpdateDrawingBrushes|UpdateFont|UpdateDpi|UpdateViewport|GetProposedFont|GetDirtyArea|GetFontSize|IsGlyphWideByFont|GetViewportInCharacters|GetViewportInPixels|SetSelectionBackground|SetAntialiasingMode|SetDefaultTextBackgroundOpacity|UpdateHyperlinkHoveredId)

...I found some more places that might need fixing:

• terminal/src/cascadia/PublicTerminalCore/HwndTerminal.cpp

  Line 475 in d7f2a39

  const auto viewInPixels = publicTerminal->_renderEngine->GetViewportInPixels(viewInCharacters);

• terminal/src/cascadia/PublicTerminalCore/HwndTerminal.cpp

  Line 498 in d7f2a39

  const auto viewInCharacters = publicTerminal->_renderEngine->GetViewportInCharacters(viewInPixels);

• terminal/src/cascadia/TerminalControl/ControlCore.cpp

  Line 1184 in d7f2a39

  return _renderEngine->GetSwapChainHandle();

• terminal/src/cascadia/TerminalControl/TermControl.cpp

  Lines 1845 to 1846 in d7f2a39

  	LOG_IF_FAILED(dxEngine->UpdateDpi(dpi));
  	LOG_IF_FAILED(dxEngine->UpdateFont(desiredFont, actualFont));

But that doesn't make this change in itself any less correct.
👉 Approve.

[miniksa]

With a bit of regexp:

(render)?\S*engine\S*->(SetCallback|SetWarningCallback|ToggleShaderEffects|SetRetroTerminalEffect|SetPixelShaderPath|SetForceFullRepaintRendering|SetSoftwareRendering|GetSwapChainHandle|Invalidate|InvalidateCursor|InvalidateSystem|InvalidateSelection|InvalidateScroll|InvalidateAll|InvalidateCircling|PrepareForTeardown|StartPaint|EndPaint|RequiresContinuousRedraw|WaitUntilCanRender|Present|ScrollFrame|PrepareRenderInfo|PaintBackground|PaintBufferLine|PaintBufferGridLines|PaintSelection|PaintCursor|UpdateDrawingBrushes|UpdateFont|UpdateDpi|UpdateViewport|GetProposedFont|GetDirtyArea|GetFontSize|IsGlyphWideByFont|GetViewportInCharacters|GetViewportInPixels|SetSelectionBackground|SetAntialiasingMode|SetDefaultTextBackgroundOpacity|UpdateHyperlinkHoveredId)

...I found some more places that might need fixing:

• terminal/src/cascadia/PublicTerminalCore/HwndTerminal.cpp

  Line 475 in d7f2a39

  const auto viewInPixels = publicTerminal->_renderEngine->GetViewportInPixels(viewInCharacters);

• terminal/src/cascadia/PublicTerminalCore/HwndTerminal.cpp

  Line 498 in d7f2a39

  const auto viewInCharacters = publicTerminal->_renderEngine->GetViewportInCharacters(viewInPixels);

• terminal/src/cascadia/TerminalControl/ControlCore.cpp

  Line 1184 in d7f2a39

  return _renderEngine->GetSwapChainHandle();

• terminal/src/cascadia/TerminalControl/TermControl.cpp

  Lines 1845 to 1846 in d7f2a39

  	LOG_IF_FAILED(dxEngine->UpdateDpi(dpi));
  	LOG_IF_FAILED(dxEngine->UpdateFont(desiredFont, actualFont));

But that doesn't make this change in itself any less correct.
👉 Approve.

The first 3 are probably "LockForReading" if they can't deal with a potentially dirty or invalid piece of data.
The last one is on a separate temporary engine that isn't associated with the main drawing pipeline so it doesn't need a lock.

But yes. Thank you.

[lhecker]

GetSwapChainHandle "technically" needs a write lock, in case that the swap chain doesn't exist yet (otherwise - theoretically - multiple threads could race and create more than one swap chain).
If we force the creation of the swap chain early in ControlCore::Initialize we can safely use a shared reading lock.
...we don't need any locks, as the swap chain is constant throughout the renderer's life, isn't it?

[miniksa]

GetSwapChainHandle "technically" needs a write lock, in case that the swap chain doesn't exist yet (otherwise - theoretically - multiple threads could race and create more than one swap chain).
If we force the creation of the swap chain early in ControlCore::Initialize we can safely use a shared reading lock.
...we don't need any locks, as the swap chain is constant throughout the renderer's life, isn't it?

I think so. It might have adjusted a bit with the multiprocess stuff. I don't want to mess with it while @zadjii-msft technically has those things in flight.

[ghost]

Hello @miniksa!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

[ghost]

🎉Windows Terminal v1.9.1942.0 has been released which incorporates this pull request.:tada:

Handy links:

• Release Notes
• Store Download

[ghost]

🎉Windows Terminal Preview v1.10.1933.0 has been released which incorporates this pull request.:tada:

Handy links:

• Release Notes
• Store Download