Weekend WTF

Whiskey_Tango_Foxtro

So a fire starts at work at the end of the week last week, and they handed me the fire hose. What I discovered probably belongs on the front page, but I'll share what I discovered here. This was one of those WTFs that lie low for a little while until they snap at you like a snake. Ouch.

Anyway, as part of our migration to a thin client system, one of our key programs, the hugest most importantest program of the suite, the place everything gets checked out at, started having problems. It's essentially a real-time data reporting system, an MDI program written in MFC that drew graphs and charts and little tables of data. And, "all of a sudden", new child windows would just go blank if you opened a more than few of them. The child windows already created would work just fine, but anything new -- blank. After some checking, we realized that they would start going blank even on regular systems, but *when* they went blank would vary, depending on color bit-depth, whether or not there were two monitors, and whether or not it was on the thin client. If you had dual monitors, for example, you might only be able to load four windows before the rest started going blank.

After the thin client guys spun their wheels for a week, trying all sorts of different window type combinations and work-arounds, they decided to delve into the code, and, since the original programmer was long gone, I got volunteered. Naturally there was no documentation on the architecture of the software and comments were as rare as a spam-free inbox.

I noticed tons of GDI object leaks, but the first thing that stood out was the OnDraw method of one of the graphs:

void OurView::OnDraw(CDC * dc)
{
  if(m_pcdcDCPointer != NULL)
  {
     delete m_pcdcDCPointer; // m_pcdcDCPointer is a private member variable points to a CDC
     m_pcdcDCPointer = NULL;
  }
  if(m_pcbitmapBitmapPointer != NULL)
  {
     delete m_pcbitmapBitmapPointer; // another member variable, points to a CBitmap
  }
  m_pcdcDCPointer = new CDC;
  m_pcbitmapBitmapPointer = new CBitmap;
  m_pcdcDCPointer->CreateCompatibleDC(dc);
  m_pcbitmapBitmapPointer->CreateCompatibleBitmap(dc, m_intWidth, m_intHeight); 
  m_pcdcDCPointer->SelectObject(m_pcbitmapBitmapPointer);
// proced to do some drawing on m_pcdcDCPointer, then bitblt back to dc.

}

Now, this is obviously a primitive attempt at double-buffering, but... wow! The original programmer obviously knew enough to delete the DC and Bitmap... but he decided to wait until the next call to OnDraw to do it! I mean, it's not like all the other windows would like a DC to draw on...

Then I checked another of the graphs and it got worse. In this one, the programmer had decided that he should instantiate m_pcdcDCPointer and m_pcbitmapBitmapPointer in the OnInitialUpdate() method and -- just for kicks -- not delete them. At all. Even in the destructor.

I think this is exactly one of those code WTFs that Alex doesn't want on the front page - and I can understand why. Namely, the code just confuses me, I don't see really anything wrong with it right away. From your explanation, it sounds like something about not deleting pointers, which is a pretty lame WTF really.

Looking at it a couple times more, it looks like he deletes some member variable pointers, but instead of doing it at the end of the function after using them, he cleans up from the last call at the start of the function. Perhaps not the best practice, but it's not a WTF, it's just boring bad code.

versatilia

@SpComb said:

I think this is exactly one of those code WTFs that Alex doesn't want on the front page - and I can understand why. Namely, the code just confuses me, I don't see really anything wrong with it right away. From your explanation, it sounds like something about not deleting pointers, which is a pretty lame WTF really.

Looking at it a couple times more, it looks like he deletes some member variable pointers, but instead of doing it at the end of the function after using them, he cleans up from the last call at the start of the function. Perhaps not the best practice, but it's not a WTF, it's just boring bad code.

erm, have you seen some of the recent front page stuff?!

ammoQ

@SpComb said:

I think this is exactly one of those code WTFs that Alex doesn't want on the front page - and I can understand why. Namely, the code just confuses me, I don't see really anything wrong with it right away. From your explanation, it sounds like something about not deleting pointers, which is a pretty lame WTF really.

Looking at it a couple times more, it looks like he deletes some member variable pointers, but instead of doing it at the end of the function after using them, he cleans up from the last call at the start of the function. Perhaps not the best practice, but it's not a WTF, it's just boring bad code.

You have to know the Windows API to see why this is a WTF. DC are "device contexts", which are in fact very limited resources; taking them without giving them back immediately after use creates a resource leak that quickly shows effects. (IMO the Real WTF(tm) is the Windows API)

HitScan

@ammoQ said:

@SpComb said:
I think this is exactly one of those code WTFs that Alex doesn't want on the front page - and I can understand why. Namely, the code just confuses me, I don't see really anything wrong with it right away. From your explanation, it sounds like something about not deleting pointers, which is a pretty lame WTF really.

Looking at it a couple times more, it looks like he deletes some member variable pointers, but instead of doing it at the end of the function after using them, he cleans up from the last call at the start of the function. Perhaps not the best practice, but it's not a WTF, it's just boring bad code.

You have to know the Windows API to see why this is a WTF. DC are "device contexts", which are in fact very limited resources; taking them without giving them back immediately after use creates a resource leak that quickly shows effects. (IMO the Real WTF(tm) is the Windows API)

I don't enjoy using it, but I understand why Win32 is so ugly, they've been trying to expand the capabilities and power of a system born in 1985 and keep it going smoothly in 2006+. (With a few file tweaks, you can run some Windows 1 or 2 programs in at least 95, if not XP. If you can recompile them, many, many more would still work.)

That said, I'm not certain that adding Ex everywhere is the best way to version library calls. :)

As for the first post: yeah, that's pretty horrid. It's a lot more than not deleting pointers on time, device contexts are system resources that are shared among the entire system. It's kind of surprising though that something that important (no DC == no drawing) is so limited, even these days.

ammoQ

@HitScan said:

I don't enjoy using it, but I understand why Win32 is so ugly, they've been trying to expand the capabilities and power of a system born in 1985 and keep it going smoothly in 2006+. (With a few file tweaks, you can run some Windows 1 or 2 programs in at least 95, if not XP. If you can recompile them, many, many more would still work.)

The Windows API is relatively insane compared to the API of the X Window system, which is approximately just as old.

HitScan

@ammoQ said:

@HitScan said:

I don't enjoy using it, but I understand why Win32 is so ugly, they've been trying to expand the capabilities and power of a system born in 1985 and keep it going smoothly in 2006+. (With a few file tweaks, you can run some Windows 1 or 2 programs in at least 95, if not XP. If you can recompile them, many, many more would still work.)

The Windows API is relatively insane compared to the API of the X Window system, which is approximately just as old.

But do they do the same amount of stuff? Just X's API doesn't do a whole lot on it's own, it requires additional layers of stuff to fill it out. Win32 is all there is. (unless you use some kind of helper lib, but even they just end up calling Win32 funcs eventually.) I don't know much about bare bones X programming, but as I understand it you can't compare it directly to Win32.

For instance: the concept of a Window Manager is internal to Windows, and so must be accessable through Win32, while in X it's just another program. (Another program that you have to know how to talk to if you want to do certain things.)

ammoQ

@HitScan said:

@ammoQ said:

The Windows API is relatively insane compared to the API of the X Window system, which is approximately just as old.

But do they do the same amount of stuff? Just X's API doesn't do a whole lot on it's own, it requires additional layers of stuff to fill it out. Win32 is all there is. (unless you use some kind of helper lib, but even they just end up calling Win32 funcs eventually.) I don't know much about bare bones X programming, but as I understand it you can't compare it directly to Win32.

The work is distributed in X, but well, an API as an API, it should be abstract so the programmer doesn't have to care.

For instance: the concept of a Window Manager is internal to Windows, and so must be accessable through Win32, while in X it's just another program. (Another program that you have to know how to talk to if you want to do certain things.)

<input type="checkbox" id="checkbox0" /><label for="checkbox0">button. </label>