Home
>
Posts
>
C++
>
The little scrollbar that could not

Maxim Zvyagintsev
Articles: 24

The little scrollbar that could not

The new Windows Terminal version has been recently released. Everything would be fine, but performance of its scrollbar leaves a great deal to be desired. Time has come to poke it and have some mumbo jumbo dances with it.

What do users usually do with a new version of any application? That's right, exactly what testers haven't done. Therefore, after a brief usage of the terminal for its intended purpose, I began to do terrible things with it. Frankly speaking, I just spilled coffee on the keyboard and accidentally clicked <Enter> when wiping it. So what happened?

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

Of course, the article heading was a huge 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 be so critical if the scrollbar worked ...

After testing a bit, I found that switching to a new line doesn't increase the buffer. Only command output does it. 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 tells us:

The output is surely impressive, so I will take advantage of the filtration power and omit everything except for the warnings containing 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); // <=Error is here
  ScrollBar().Minimum(0);
  ScrollBar().Value(0);
  ScrollBar().ViewportSize(bufferHeight);
  ....
}

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

No doubts, simulating the scroll height is great, but why do we set 0 as the maximum? After referring to the documentation, it became clear that the code is not very suspicious. Don't get me wrong: indeed, subtracting a variable from itself is suspicious, but we get zero at the output, which does not do any harm. In any case, I tried to specify the default value (1) in the Maximum field:

The scrollbar appeared, but it still does not work:

Just in case, then I was holding <Enter> for about 30 seconds. Apparently this was not the problem, so I left it as it was, except for replacing bufferHeight – bufferHeight with 0: 

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

  ScrollBar().Maximum(0); // <= Here is the replacement
  ScrollBar().Minimum(0);
  ScrollBar().Value(0);
  ScrollBar().ViewportSize(bufferHeight);
  ....
}

So, I wasn't actually getting close to solving the problem. In the absence of a better offer, let's move on to the debugging part. First, we could set 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 Viewport offset relatively to the buffer.

Let me tell you a bit about the internals of this scrollbar (and most likely about others as well). We have one big buffer that stores all the output. To interact with it, some kind of abstraction is used for printing on the screen, in this case, it's viewport.

Using these two primitives, we can become aware of what our problem is. Transition to the new line doesn't increase the buffer, that's why we simply have nowhere to go. Therefore, the problem is right in it.

Armed with this commonplace knowledge, we continue our heroic debugging. After a little walk around the function, this fragment got my attention:

// 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 configured the ScrollBar above, let's move on to 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. Let's set breakpoints on them and find where they change. Well, I'll cheat here with _viewport and set the breakpoint not at the variable itself but at its field top, which we actually need.

Now we're pressing <F5>, but nothing is happening ... Ok, then let's press <Enter> a couple of dozen times. Nothing happened. Apparently, we set the breakpoint at _buffer too recklessly. _viewport remained at the top of the buffer, which didn't increase in size.

In this case, it makes sense to enter a command to renew the _viewport top. After that we stopped at 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 left a comment where we stopped. If you look at the comment in the fragment, it becomes clear that we are closer to the solution than ever. It's in this place where the visible part is shifted relatively to the buffer, and we can scroll. Having observed this 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 viewport; therefore, we don't get it down 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 will happen:

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;
    }
  }
  ....
}

The result of this run:

Miracles! I pressed Enter a number of times, and the scrollbar works. Well, until we enter something... To demonstrate this fail, here's a gif file:

It seems that we're 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
  cursor.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 do we get?

Well, that's better. However, there is a problem: we shift the output point, but don't 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 debugging:

// 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>?

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

Fine, enough of dragging it out - the entire buffer, but without the first line. And this is a considerable problem, because it is precisely the reason why we are losing history, moreover, fragmentarily. This is what our help output snippet will look like after moving to a new line:

I left an arrow at the place with "LONG DESCRIPTOIN". Maybe then overwrite the buffer with an offset of one line? This would have worked if this callback had not been called every single time.

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

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

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

Conclusion

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 a variable subtraction from itself strongly encouraged me, and I vigorously proceeded to write the text. However, as you can see, we haven't been out of the woods yet and everything turned out to be much more complicated.

So I decided to stop. I could have spent another couple of evenings, but the deeper I was going, the more problems emerged. 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 hadn't finished the research and it was interesting for you to take a walk with me along the insides 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 for a month. If you haven't tried the PVS-Studio static analyzer in practice yet, this is a good reason to do it. Just enter "#WindowsTerminal" into the "Message" field on the download page.

In addition, taking the opportunity, I'd like to remind you that soon there will be a version of the C# analyzer working under Linux and macOS. Right now you can sign up for beta testing.