Get best of the week

Scrollbar that failed


Recently released a new version of Windows Terminal. Everything would be fine, but the performance of her scrollbar left much to be desired. Therefore, it is time to stick a little stick at him and play the tambourine.

What do users usually do with the new version of any application? That's right, exactly what testers did not do. Therefore, after a brief use of the terminal for its intended purpose, I began to do terrible things with it. Alright, alright, I just spilled coffee on the keyboard and accidentally clamped <Enter> when I wiped it. What happened in the end?



Yes, it does not look very impressive, but do not rush to throw stones at me. Pay attention to the right side. First try to figure out what is wrong with her. Here's a screenshot for a hint:


Of course, the title of the article was a serious spoiler. :)

So, there is a problem with the scrollbar. Moving to a new line many times, after crossing the lower border, you usually expect a scrollbar to appear, and you can scroll up. However, this does not happen until we write a command with the output of something. Let's just say the behavior is strange. However, this might not have been so critical if the scrollbar worked ...

Having tested a little, I found that switching to a new line does not increase the buffer. This only makes the output of commands. So the above whoami will increase the buffer by only one line. Because of this, over time we will lose a lot of history, especially after clear.

The first thing that came to my mind was to use our analyzer and see what it says:


The conclusion, of course, is of impressive size, so I will take advantage of the power of filtering and crop everything except the one containing the ScrollBar :


I can’t say that there are a lot of messages ... Well, maybe then there is something related to the buffer?


The analyzer did not fail and found something interesting. I highlighted this warning above. Let's see what is wrong there:

V501 . There are identical sub-expressions to the left and to the right of the '-' operator: bufferHeight - bufferHeight TermControl.cpp 592

bool TermControl::_InitializeTerminal()
{
  ....
  auto bottom = _terminal->GetViewport().BottomExclusive();
  auto bufferHeight = bottom;

  ScrollBar().Maximum(bufferHeight - bufferHeight); // <=  
  ScrollBar().Minimum(0);
  ScrollBar().Value(0);
  ScrollBar().ViewportSize(bufferHeight);
  ....
}

This code is accompanied by a comment: "Set up the height of the ScrollViewer and the grid we're using to fake our scrolling height."

Simulating the scroll height is, of course, good, but why are we putting 0 at the maximum? Turning to the documentation , it became clear that the code is not very suspicious. Do not get me wrong: subtracting a variable from itself is, of course, suspicious, but we get a zero at the output, which does not harm us. In any case, I tried to specify the default value (1) in the Maximum field :


The scrollbar appeared, but it also does not work:



If anything, then I clamped <Enter> for 30 seconds. Apparently this is not the problem, so let’s leave it as it was, except by replacing bufferHeight - bufferHeight with 0:

bool TermControl::_InitializeTerminal()
{
  ....
  auto bottom = _terminal->GetViewport().BottomExclusive();
  auto bufferHeight = bottom;

  ScrollBar().Maximum(0); // <=   
  ScrollBar().Minimum(0);
  ScrollBar().Value(0);
  ScrollBar().ViewportSize(bufferHeight);
  ....
}

So, we are not particularly close to solving the problem. In the absence of a better offer to go into debag. At first we could put a breakpoint on the changed line, but I doubt that it will help us somehow. Therefore, we first need to find the fragment that is responsible for the offset of the Viewport relative to the buffer.

A little about how the local (and most likely any other) scrollbar works. We have one big buffer that stores all the output. To interact with it, some kind of abstraction is used to draw on the screen, in this case, viewport .

Using these two primitives, we can understand what our problem is. Going to a new line does not increase the buffer, and because of this, we simply have nowhere to go. Therefore, the problem is in it.

Armed with this commonplace knowledge, we continue our heroic debag. After a little walk around the function, I drew attention to this fragment:

// This event is explicitly revoked in the destructor: does not need weak_ref
auto onReceiveOutputFn = [this](const hstring str) {
  _terminal->Write(str);
};
_connectionOutputEventToken = _connection.TerminalOutput(onReceiveOutputFn);

After we have configured the ScrollBar above , we configure various callback functions and execute __connection.Start () for our newly minted window. After which the above lambda is called. Since this is the first time we are writing something to the buffer, I suggest starting our debug from there.

We set a breakpoint inside the lambda and look in _terminal :



Now we have two variables that are extremely important to us - _buffer and _mutableViewport . Put breakpoints on them and find where they change. True, with _viewport I’ll cheat a little and put a breakpoint not on the variable itself, but on its top field (we just need it).

Now click on <F5>, and nothing happens ... Well, then let's hit a couple of dozen times <Enter>. Nothing happened. Apparently, on _buffer we set a breakpoint too recklessly, and _viewport , as expected, remained at the top of the buffer, which did not increase in size.

In this case, it makes sense to enter a command to cause a vertex update._viewport . After that, we got up on a very interesting piece of code:

void Terminal::_AdjustCursorPosition(const COORD proposedPosition)
{
  ....
  // Move the viewport down if the cursor moved below the viewport.
  if (cursorPosAfter.Y > _mutableViewport.BottomInclusive())
  {
    const auto newViewTop =
      std::max(0, cursorPosAfter.Y - (_mutableViewport.Height() - 1));
    if (newViewTop != _mutableViewport.Top())
    {
      _mutableViewport = Viewport::FromDimensions(....); // <=
      notifyScroll = true;
    }
  }
  ....
}

I pointed out a comment where we left off. If you look at the commentary on the fragment, it becomes clear that we are closer to the solution than ever. It is in this place that the visible part is shifted relative to the buffer, and we get the opportunity to scroll. Having observed the behavior a bit, I noticed one interesting point: when moving to a new line, the value of the cursorPosAfter.Y variable is equal to the value of the viewport , so we don’t omit it and nothing works. In addition, there is a similar problem with the newViewTop variable . Therefore, let's increase the value of cursorPosAfter.Y by one and see what happened:

void Terminal::_AdjustCursorPosition(const COORD proposedPosition)
{
  ....
  // Move the viewport down if the cursor moved below the viewport.
  if (cursorPosAfter.Y + 1 > _mutableViewport.BottomInclusive())
  {
    const auto newViewTop =
      std::max(0, cursorPosAfter.Y + 1 - (_mutableViewport.Height() - 1));
    if (newViewTop != _mutableViewport.Top())
    {
      _mutableViewport = Viewport::FromDimensions(....); // <=
      notifyScroll = true;
    }
  }
  ....
}

And the launch result:


Wonders! I entered ne quantity and the scrollbar works. True, until the moment we introduce something ... To demonstrate the file, I will attach a gif:


Apparently, we are doing a few extra jumps to a new line. Let's then try to limit our transitions using the X coordinate. We will only shift the line when X is 0:

void Terminal::_AdjustCursorPosition(const COORD proposedPosition)
{
  ....
  if (   proposedCursorPosition.X == 0
      && proposedCursorPosition.Y == _mutableViewport.BottomInclusive())
  {
    proposedCursorPosition.Y++;
  }

  // Update Cursor Position
  ursor.SetPosition(proposedCursorPosition);

  const COORD cursorPosAfter = cursor.GetPosition();

  // Move the viewport down if the cursor moved below the viewport.
  if (cursorPosAfter.Y > _mutableViewport.BottomInclusive())
  {
    const auto newViewTop =
      std::max(0, cursorPosAfter.Y - (_mutableViewport.Height() - 1));
    if (newViewTop != _mutableViewport.Top())
    {
      _mutableViewport = Viewport::FromDimensions(....);
      notifyScroll = true;
    }
  }
  ....
}

The fragment written above will shift the Y coordinate for the cursor. Then we update the cursor position. In theory, this should work ... What happened?



Well, of course, better. However, there is a problem in that we shift the output point, but do not shift the buffer. Therefore, we see two calls of the same command. It may, of course, seem that I know what I am doing, but this is not so. :)

At this point, I decided to check the contents of the buffer, so I returned to the point at which I started the debug:

// This event is explicitly revoked in the destructor: does not need weak_ref
auto onReceiveOutputFn = [this](const hstring str) {
  _terminal->Write(str);
};
_connectionOutputEventToken = _connection.TerminalOutput(onReceiveOutputFn);

I set a breakpoint in the same place as last time, and started looking at the contents of the str variable . Let's start with what I saw on my screen:


What do you think will be in the str string when I press <Enter>?

  1. String "LONG DESCRIPTION" .
  2. The entire buffer that we now see.
  3. The entire buffer, but without the first line.

I will not languish - the entire buffer, but without the first line. And this is a considerable problem, because it is precisely because of this that we are losing history, moreover, pointwise. This is how our help output fragment will look after going to a new line:


With an arrow I marked the place where “LONG DESCRIPTOIN” was . Maybe then overwrite the buffer with an offset of one line? This would work if this callback was not called for every sneeze.

I have discovered at least three situations when it is called,

  • When we enter any character;
  • When we move through history;
  • When we execute the command.

The problem is that you need to move the buffer only when we execute the command, or enter <Enter>. In other cases, doing this is a bad idea. So we need to somehow determine inside what needs to be shifted.

Conclusion


Picture 18


This article was an attempt to show how skillfully PVS-Studio was able to find defective code leading to the error I noticed. The message on the topic of subtracting a variable from itself strongly motivated me, and I vigorously proceeded to write the text. But as you can see, my joy was premature and everything turned out to be much more complicated.

So I decided to stop. One could still spend a couple of evenings, but the longer I did this, the more and more problems arose. All I can do is wish the Windows Terminal developers good luck in fixing this bug. :)

I hope I didn’t disappoint the reader that I didn’t finish the research and it was interesting for me to take a walk along the inside of the project. As a compensation, I suggest using the #WindowsTerminal promo code, thanks to which you will receive a demo version of PVS-Studio not for a week, but immediately for a month. If you have not tried the PVS-Studio static analyzer in practice, this is a good reason to do just that. Just enter "#WindowsTerminal" in the "Message" field on the download page .

And also, taking this opportunity, I want to remind you that soon there will be a version of C # analyzer working under Linux and macOS. And now you can sign up for preliminary testing.


If you want to share this article with an English-speaking audience, then please use the link to the translation: Maxim Zvyagintsev. The Little Scrollbar That Could Not .

More articles:


All Articles