#494 RestoreDC causes TDC state corruption and crash

[Vidar Hasfjord]

TDC::RestoreDC has a horrible bug, which has lingered since OWL 5, at least. It does not play nice with the "orphan control" performed by TDC. In fact, RestoreDC is just a thin wrapper of the Windows API function RestoreDC. It is completely oblivious to the state tracking done by TDC as part of the orphan control.

This means that any references that may have been acquired by TDC, to the objects deselected by RestoreDC, will not be released, and references to the objects reselected by RestoreDC will not be acquired. There may hence be undetected orphans, while detected orphans will dangle and pile up. The following simple loop crashes in my test at about 10000 iterations:

for (...)
{
  const auto stateId = dc.SaveDC();
  const auto pen = TPen{...};
  dc.SelectObject(pen);
  dc.MoveTo(...);
  dc.LineTo(...);
  dc.RestoreDC(stateId);
}

I find this issue quite puzzling. The pair of functions SaveDC and RestoreDC are very useful in nested graphics routines. Similar functions for saving and restoring system state are often found in other graphics systems (well, that is what I remember from what I was taught in college back around 1990). You would think they were often used. But I cannot find any mention in the discussion forum, nor in the mailing list, about their usage or any problems. No bug tickets. Nothing at all. Strange.

An Internet search found this post from 2001 in a discussion about "Object Destruction in MFC" by Doug Harris, who seems to have caught on to the problem: "OWL reference-counted its C++ GDI objects, which I thought was a fine idea. The only problem I recall (it's been 5 years) was that it didn't do anything special for SaveDC/RestoreDC, and I believe it needed to". Why, Doug, did you not file a bug report with OWLNext? I also found this post from 1998 at ExpertsExchange by Quinlan, who reports a crash issue with a code segment using SaveDC and RestoreDC (the reply/solution is not public, though). I also got many hits about SaveDC and RestoreDC in OWL teaching material, so you would think the functions would be in use.

Now that I have rediscovered this issue, I remember running into it as a young programmer over 25 years ago. This was long before OWLNext and my participation online, so I didn't seek any help. Not having figured out what the issue was, I stopped using SaveDC and RestoreDC altogether, and I haven't used these functions since — until now, as I seek to better understand and improve upon TDC. See [discussion:00f93e1a3a].

[Vidar Hasfjord]

Note that calling TDC::RestoreObjects before TDC::RestoreDC will prevent orphans piling up and the program crashing. However, the orphan control is still messed up, since references to the reselected objects are not acquired, meaning a handle may be deleted despite still being selected.

[Vidar Hasfjord]

This issue was fixed in Owlet in [r5445] as follows:

void TDC::RestoreDC(int savedStateStackIndex)
{
  const auto deselectedPen = GetCurrentObject<HPEN>();
  const auto deselectedBrush = GetCurrentObject<HBRUSH>();
  const auto deselectedFont = GetCurrentObject<HFONT>();
  const auto deselectedPalette = GetCurrentObject<HPALETTE>();
  const auto deselectedBitmap = GetCurrentObject<HBITMAP>();

  const auto ok = ::RestoreDC(GetHDC(), savedStateStackIndex);
  if (!ok) throw TXGdi{GetHDC(), "RestoreDC failed"};

  const auto restoreRef = [&](auto deselectedHandle, auto orgHandle)
  {
    OWL_CHECK(deselectedHandle);
    const auto restoredHandle = GetCurrentObject<decltype(deselectedHandle)>();
    OWL_CHECK(restoredHandle);
    if (restoredHandle == deselectedHandle) return; // Nothing to do.

    // We do not want to take up a reference to an original handle, i.e. a handle that was selected
    // before the TDC was constructed, because TDC does not do that at construction.
    //
    // A null orgHandle implies that the restored handle is the original value. Either it has never
    // been changed by a SelectObject call, or it has been restored by a call to the corresponding
    // restore function (RestorePen, etc.) or RestoreObjects (which restores all), and there has
    // been no calls to SelectObject since.
    // 
    if (orgHandle && restoredHandle != orgHandle)
      TGdiObject::RefInc(restoredHandle); // Take up a reference to the restored handle.
    TGdiObject::RefDec(deselectedHandle, false); // Give up our reference to the deselected handle.
  };
  restoreRef(deselectedPen, OrgPen);
  restoreRef(deselectedBrush, OrgBrush);
  restoreRef(deselectedFont, OrgFont);
  restoreRef(deselectedPalette, OrgPalette);
  restoreRef(deselectedBitmap, OrgBitmap);
}

Please review. If you can fault my logic, let me know. I would like this fix to be robust, so any feedback would be appreciated.