Crash durch unsachgemäßen Umgang mit PreTranslateMessage

Neulich bei einem Codereview, lief mir Code in den beiden nachfolgenden Formen über meinen Monitor:

BOOL CMyDialog::PreTranslateMessage(MSG *pMsg)
{
    // Translate Messages (ON_KEYDOWN -> ON_COMMAND)
    TranslateAccelerator(m_hWnd,m_hAccel,pMsg);
    // Let the ToolTip process this message.
    m_tooltip.RelayEvent(pMsg);
    return __super::PreTranslateMessage(pMsg);
}

– bzw.  –

BOOL CMyWnd::PreTranslateMessage(MSG *pMsg)
{
  BOOL bResult = __super::PreTranslateMessage(pMsg);
  DoSomething();
  return bResult;
}

Sieht OK aus, aber hier tritt ein grundsätzliches Problem auf:

In beiden Funktionen wird evtl. eine Nachricht behandelt. Allerdings wird in diesem Fall nicht umgehend die Funktion verlassen. Durch das Behandeln der Nachricht kann nämlich das Fenster/Objekt, zu dem PreTranslateMessage gehört, bereits zerstört sein. In diesem Fall kehrt PreTranslateMessage zurück und der this Zeiger ist bereits ungültig.

Es ist also imminent wichtig in dem Moment in dem erkannt wird, dass die Nachricht behandelt wurde, auch umgehend die Funktion mit TRUE zu verlassen und keine weitere Memberfunktion oder gar Membervariable mehr zu nutzen. Beides könnte zu einem üblen Crash führen.

Der korrekte Code sähe also so aus:

BOOL CMyDialog::PreTranslateMessage(MSG *pMsg)
{
  // Translate Messages (ON_KEYDOWN -> ON_COMMAND)
  if (TranslateAccelerator(m_hWnd,m_hAccel,pMsg))
    return TRUE;
  // Let the ToolTip process this message.
  m_tooltip.RelayEvent(pMsg);
  return __super::PreTranslateMessage(pMsg);
}

– oder –

BOOL CMyWnd::PreTranslateMessage(MSG *pMsg)
{
  if (__super::PreTranslateMessage(pMsg))
    return TRUE;
  DoSomething();
  return FALSE;
}

PS: Beide Codeteile wurden durch Crashdumps aus WinQual gefunden. Regelmäßig, alle 2 Monate schaue ich mir Dumps an, von den Top-Crashes, die dort verzeichnet sind, und mache entsprechende Code-Reviews.
Der erste Code, stammte aus einem speziellen nicht modalen Dialog, der durch Drücken bestimmter Tastenkombinationen geschlossen und zerstört wurde.
Der zweite Code stammte aus einem Popup-Fenster, dass auch durch Mausaktivitäten oder Tastendrücke zerstört wurde.
Ich kann jedem nur raten WinQual auch zu nutzen, es dient der Qualtitätssicherung und man findet viele kleine Bugs, die manchen User ärgern, die aber nie sonst gemeldet würden.

VS-Tipps & Tricks: Direkter Break in den Debugger bei einem ASSERT

ASSERTs in der MFC und in der CRT sind tolle Hilfsmittel, aber nicht selten verfälschen sie auch das Problem alleine dadurch, dass ein Fenster aufpoppt, wenn der ASSERT zuschlägt. Hat man nun einen Code, der in einem Tooltipp etwas Böses macht, dann wird der Tooltipp selbst aber schon wieder durch das erscheinen der ASSERT Meldung zerstört. Oder es wird ein neuer ASSERT ausgelöst. Der Callstack wird dadurch oft schwer zu lesen.
Besonders heikel kann dies auch noch werden wenn man mehrere Threads hat. Gleichfalls problematisch ist, dass in dem Moment in dem die ASSERT Box auftaucht nun auch wieder alle Timer weiterlaufen und sehr eigentümliche Seiteneffekte weiter auslösen können, dito. Probleme in WM_PAINT Handlern, denn auch die lösen evtl. schon wieder Aktionen aus, die Variablen verändern.

Nett ist am ASSERT-Dialog natürlich die Möglichkeit Ignorieren zu sagen und das Programm weiter laufen zu lassen. Ganz besonders wenn man Debug Versionen im Testfeld mit Anwendern testet.

Dennoch bin ich bei Debug-Versionen dazu übergegangen ASSERTs direkt  crashen zu lassen, bzw. direkt einen Debug-Break auszulösen. Das erleichtert das Lesen des Crashdumps bzw. hilft auch beim Debuggen, weil man direkt an der Stelle steht wo es hakt und alle Fenster und Variableninhalte exakt noch so sind, wie Sie es beim Auftreten des Problems waren (Tooltips, Popups, Menüs etc.).

Der Code um das zu erreichen ist relativ simpel. Man verwendet dazu _CrtSetReportHook2. In dem Hook sagt man einfach was man gerne hätte. Nämlich bei einem ASSERT oder ERROR keinen Dialog sondern einen Break (INT3).

#ifdef _DEBUG
int __cdecl DebugReportHook(int nReportType, char* , int* pnRet)
{
  // Stop if no debugger is loaded and do not assert, cause a crash
  // - returning TRUE indicates that we handled the problem, so no other hook
  //   needs to perform any action
  // - setting the target of *pnRet to TRUE indicates that the CRT should
  //   execute an INT3 and should crash or break into the debugger.
  return *pnRet = nReportType==_CRT_ASSERT ||
                  nReportType==_CRT_ERROR ?
                            TRUE : FALSE;
}
#endif

void SetBreakOnAssert(BOOL bBreakOnAssert/* =FALSE */)
{  
// Need to disable the ASSERT handler?
#ifdef _DEBUG  
  if (bBreakOnAssert)   
    _CrtSetReportHook2(_CRT_RPTHOOK_INSTALL, DebugReportHook); 
  else   
    _CrtSetReportHook2(_CRT_RPTHOOK_REMOVE, DebugReportHook);
#else
  UNUSED_ALWAYS(bBreakOnAssert);
#endif
}

Durch diese kleine Funktion SetBreakOnAssert kann man dieses Verhalten nun einfach ein- und ausschalten. Nähere Details stehen im Kommentar der Hook-Funktion.

CMapStringTo… HashKey Implementierung in VC-2003/5/8 ist auch fragwürdig

 Hash-Algorithmen haben es mir irgendwie angetan. 1984 hatte ich das erste mal mit einem miesen Hashverfahren zu tun, das einfach versagte wenn es nur um Ziffern ging. Das ganze betraf das Betriebssystem OASIS (später TheOS) und dessen Index Dateiorganisation. Ich entwickelte hierfür einen Patch in Assembler, der damals exakt in den x-Bytes der Kernels in Z80 Assembler passen musste.
Das Problem bei Strings die nur aus Ziffern bestehen ist das die Veränderung, die immer nur die unteren 4 Bits betrifft und die höheren 4 Bits immer konstant sind mit 3 belegt sind.

Als ich gelesen habe,  dass in VS-2010 die CMap Implementierung nun auch für Strings den selben Algorithmus aus der STL erhalten, habe ich mir auch den mal genauer angesehen. (siehe Beitrag Die MFC erhält mit VC-2010 jetzt eine neue HashKey Implementierung)
Der sieht so aus:

inline UINT CMapStringToString::HashKey(LPCTSTR key) const
{
  UINT nHash = 0;
  if (key == NULL)
  {
    AfxThrowInvalidArgException();
  }
 
  while (*key)
    nHash = (nHash<<5) + nHash + *key++;
  return nHash;
}

Und auch in diesem Fall muss ich sagen, dass der Algorithmus nur scheinbar intelligent ist. Wenn man sich aber die Abstände die hier erzeugt werden etwas genauer anschaut kommt man schnell auf eine Wertekombination, in der der Algorithmus versagt.
Wie sehr er versagt zeigt das folgende Beispiel, in dem ich einfach einen String verwende der aus 5 Ziffern besteht und immer den Abstand 11 verwendet.

CMapStringToPtr myMap;
for (int i=0; i<1000; i+=11)
{
  CString strKey;
  strKey.Format(_T("%05d"),i);
  myMap[strKey] = NULL;
}

Das erschreckende Ergebnis der Verteilung ist bei einem Unicode wie auch MBCS Projekt:

TMyMap<class CMapStringToPtr>::DumpMap
Bucket  0 =  0
Bucket  1 =  0
Bucket  2 =  0
Bucket  3 =  0
Bucket  4 =  0
Bucket  5 =  0
Bucket  6 =  0
Bucket  7 =  0
Bucket  8 = 36
Bucket  9 =  0
Bucket 10 =  0
Bucket 11 =  0
Bucket 12 =  0
Bucket 13 =  0
Bucket 14 = 55
Bucket 15 =  0
Bucket 16 =  0

Wie gut, dass auch die String Variante von HashKey überarbeitet wird.

BTW: Da die internen Strukturen protected sind muss man zu einem kleinen Trick greifen um die Verteilung ausgeben zu können. Ich habe dazu ein template verwendet, dass für alle MFC Maps funktioniert.

template<class TBase> class TMyMap : public TBase
{
public:
  void DumpMap()
  {
    TRACE(__FUNCTION__ "\n");
    if (m_pHashTable)
    {
      for (size_t i=0; i<m_nHashTableSize; ++i)
      {
        int iCount = 0;
        for (CAssoc *p = m_pHashTable[i]; p; p=p->pNext)
          ++iCount;
        TRACE("Bucket %2d = %4d\n", i, iCount);
      }
    }
  }
};

Merke: Wer Hashes verwendet sollte sich über sein Hashverfahren wirklich gedanken machen.

BTW: Sollte es den Leser meines Blogs langsam langweilen weil ich mich permanent mit Hash-Algorithmen aufhalte, der sei getröstet: Dies war vorläufig mein letzter Beitrag zu CMap… :mrgreen:

Die MFC erhält mit VC-2010 jetzt eine neue HashKey Implementierung

Ich hatte die schlechte Hashkey Implementierung ja bereits in VC-2005 als Bug gemeldet (siehe Die HashKey Implementierung in der MFC in VC-2005 und VC-2008). In VC-2008 geschah dies bzgl. wieder nichts. Aber was lange währt wird manchmal gut 😉

Die neue Implementierung wird aus der aktuellen STL übernommen. Diese Implementierung sorgt für Integer und Pointer für eine sehr gute zufällige Verteilung. Und weitere gute Nachricht dazu, auch die Implementierung für Strings wird aus der STL übernommen.

Zitat:

Hello Martin,

Thanks for the report. This issue has been fixed in MFC for Visual Studio 2010. MFC now uses the STL hash functions directly when possible, or uses a new algorithm copied from STL (in the case of strings).

Pat Brenner
Visual C++ Libraries Development
Von Microsoft am 20.07.2009 um 12:15 bereitgestellt

Siehe https://connect.microsoft.com/VisualStudio/feedback/ViewFeedback.aspx?FeedbackID=468860

Die Funktion ReportFault unter Vista kehrt nicht mehr zurück, entgegen der Dokumentation

Wer unter Windows XP angefangen die FaultRep.dll für Crash-Reports zu verwenden, wird unter Vista eine üble Erfahrung machen. Entgegen der Dokumentation kehrt ReportFault unter Vista nicht zurück.

Das ist besonders lästig, wenn man nach dem Melden des Fehlers aufgrund des Feedbacks des Kunden noch selber Aktivitäten in der eigenen Software vorgesehen hatte.

Wieder ein Fall wo es schwierig ist zwischen verschiedenen Windows-Betriebssystemen kompatibel zu bleiben. Da hat man sich auf Windows XP eingelassen und unter Vista ist das entsprechende Interface schon wieder deprecated.

Tipps & Tricks: Wie man einen permanenten ASSERT direkt mit einem Kommentar versieht

Ich verwende gerne ASSERT’s in meinem Code.
Sie sind ein wirksames Mittel Zustände abzufragen und bereist in der Testphase unzulässige Konstellationen oder Funktionsaufrufe zu entdecken.

Nun gibt es ja auch if, else oder switch Blöcke an denen das Programm normalerweise nicht vorbei kommen sollte. So eine Stelle versieht man dann schon mal mit einem

_ASSERT( FALSE );
// oder wer die MFC benutzt eben ASSERT,
// obwohl dies auch nur ein Synonym für den CRT _ASSERT makro ist
ASSERT(FALSE);

Jetzt müsste man noch einen Kommentar davor setzen, damit klar wird was hier schief läuft. Man kann das Ganze aber auch einfach kombinieren und noch einen Zusatznutzen erreichen indem man den etwas unbekannteren Makro _ASSERTE verwendet:

_ASSERTE( !"MyFuncFooHandler: This is not allowed in my special Foo Handler" );

Die Negation macht aus dem Zeiger auf den konstanten String FALSE, und damit schlägt der ASSERT an.
Wenn man jetzt wie hier gezeigt noch den _ASSERTE Makro verwendet, dann wird diese Expression, also der Text, sofort mit anzeigt. Man sieht dann sofort was das Problem ist sobald der ASSERT Dialog angezeigt wird.

VC-2010: Breaking Changes in der STL

Ich habe mein erstes größeres Projekt mal testweise in VS-2010 übernommen und bin sofort über ein Problem in der STL gestolpert. Ein std::set<FOO>::iterator liefert in VC-2010 jetzt eine const FOO & Referenz!

Das Ändern von Objekten in einem std::set war sowieso nur erlaubt, wenn sich die Reihenfolge nicht ändert.

Die Folge ist, dass der nachfolgende Code in VC-2010 nicht mehr kompiliert.

#include <set>
struct S_FOO
{
    unsigned long     m_dw1, m_dw2;
    // simplified sample without bool operator<
};

class CMySet : public std::set<S_FOO>
{
public:
    void SomethingSpecial()
    {
        // Order must not be changed!
        for (iterator it=begin(); it!=end(); ++it)
            it->m_dw1 = 0;   // <----- C3892!
    }
};

int _tmain(int argc, _TCHAR* argv[])
{
    CMySet myset;
    myset.SomethingSpecial();
    return 0;
}

Man erhält den Fehler C3892: ‚it‘ : you cannot assign to a variable that is const

Das und noch einige andere Breaking Changes  wurden gestern in VC-Blog veröffentlicht:

http://blogs.msdn.com/vcblog/archive/2009/05/25/stl-breaking-changes-in-visual-studio-2010-beta-1.aspx

MSDN Abonnenten können ab Montag den 18.05.2009 Visual Studio 2010 Beta 1 herunterladen

Jihad Dannawi kündigt in seinem Blog die Veröffentlichung von Visual Studio 2010 Beta 1 für MSDN Subscriber an:
http://blogs.msdn.com/dannawi/archive/2009/05/15/visual-studio-2010-beta-1-available-for-the-msdn-subscribers-on-monday-may-18th.aspx

Dito kann man es auf ZDNet lesen:
http://blogs.zdnet.com/microsoft/?p=2769

Ich freue mich schon drauf, ich hoffe es findet sich Zeit mal wirklich damit spielen zu können und um herauszufinden ob der Slogan „The new 10 is the next 6“ wirklich trägt… 😉

Refactoring mit Hilfe des Compilers kann eine tückische Sache werden

Wieder mal eine nette Falle: Implizite Konvertierungen und ein Refactoring-Versuch.

Folgende Methoden wurden in einer Klasse verwendet:

...
bool GetTableCoreData(long lIdAddrSet,
            CAgvipTableCoreData &coreData,
            bool bSilent=false);
bool GetTableCoreData(long lIdAddrSet, long lIdProject,
            CAgvipTableCoreData &coreData,
            bool bSilent=false);
bool GetTableCoreData(long lIdAddrSet,
            CDataConnection &dataConnection,
            CAgvipTableCoreData &coreData);
...

Die dritte Methode passte mir nicht von der Reihenfolge der Argumente. und ich änderte sie wie folgt um:

bool GetTableCoreData(long lIdAddrSet,
            CAgvipTableCoreData &coreData,
            CDataConnection &dataConnection);

Ich habe mich nun einfach darauf verlassen, dass der Compiler mir alle entsprechenden Code Stellen schon anmeckern wird, an denen hier was nicht passt. Da ich noch einiges anderes an der Klasse geändert hatte, dauerte es noch eine Weile bis ich den nächsten Build angeworfen habe, und ehrlich gesagt, habe ich das Refactoring dieser Funktion vergessen.
Typischer Fall von: Zu viel auf einmal & Der Compiler macht einfach nicht was ich will 😉

Was passierte? Nichts ❗
Ich bekam keine Fehlermeldung zu dieser Änderung, denn CDataConnection hat eine implizite Konvertierung auf bool. Die Folge war, dass die erste Signatur der Funktion auch dieser Folge von Argumenten entsprach.

bool GetTableCoreData(long lIdAddrSet,
            CAgvipTableCoreData &coreData,
            bool bSilent=false);

Logisch, dass diese Funktion natürlich eine anderes Verhalten hatte und hier nicht mehr das passierte was ich eigentlich wollte.
Dämlicherweise rutschte diese Änderung auch noch durch die Tests und eine ganze Funktionsgruppe unserer Software wurde lahmgelegt und so ausgeliefert… Ein Bug, dazu noch von der Kategorie vermeidbar.
Was lernen wir:

  1. Es gibt keine fehlerfreie Software!
  2. Die kleinen Änderungen bringen die größten Fehler!
  3. Sich beim Refactoring auf den Compiler zu verlassen kann tückisch werden!

Das Web Browser Control stiehlt den Fokus wenn ein Dokument geladen wurde

Wenn man ein Web Browser Control einbindet und dieses eine Seite lädt, dann wird der Fokus in dieses Browser Control gesetzt. Dagegen ist kein Kraut und keine Notification gewachsen.
Genaugenommen ist nicht das Webbrowser Control schuld, sondern der Scriptcode der auf der Seite läuft und den Fokus umsetzt. Das sieht man schnell wenn man den Moment der WM_KILLFOCUS Nachricht im Debugger abpasst und sich den Stacktrace ansieht.

0013b84c 4a570824 USER32!NtUserSetFocus 
0013b858 4a5ce628 mshtml!CDoc::TakeFocus+0x2a 
0013b880 4a63fc0b mshtml!CElement::BecomeCurrent+0x167 
0013b8b4 4a63fb72 mshtml!CElement::focusHelper+0xcc 
0013b8c0 4a587c85 mshtml!CElement::focus+0x1d 
0013b8cc 4a5d7477 mshtml!Method_void_void+0x17 
0013b94c 4a57fae8 mshtml!CBase::ContextInvokeEx+0x462 
0013b97c 4a575413 mshtml!CElement::ContextInvokeEx+0x72 
0013b9b0 76fa5295 mshtml!CElement::ContextThunk_InvokeEx+0x44 
0013b9e8 76fa5208 jscript!IDispatchExInvokeEx2+0xa9 
0013ba20 76fa5323 jscript!IDispatchExInvokeEx+0x56 
0013ba90 76fa577b jscript!InvokeDispatchEx+0x78 
0013bad8 76fa57c6 jscript!VAR::InvokeByName+0x1c1 
0013bb18 76fa4ab0 jscript!VAR::InvokeDispName+0x43 
0013bb3c 76fa5a14 jscript!VAR::InvokeByDispID+0xfb 
0013bd30 76fa46d8 jscript!CScriptRuntime::Run+0x195b 
0013bdf4 76fa506e jscript!ScrFncObj::Call+0x69 
0013be6c 76fa5f6a jscript!CSession::Execute+0xb8 
0013bf6c 76fa672f jscript!NameTbl::InvokeDef+0x183 
0013c040 76fa5295 jscript!NameTbl::InvokeEx+0xd2

Dummerweise gibt es kein Event mehr, das danach gefeuert wird, wenn der Skript-Code abläuft. Das letzte Event bevor das aktive Fenster den Fokus verliert ist OnDocumentComplete.

Es gibt auch einige Threads die dieses Thema behandeln, aber keine vernünftige Lösung. Von so manchen Timerlösungen halte ich nichts, die da so vorgeschlagen werden, wer weiß schon wann eine Seite geladen ist?
Besonders ärgerlich auch, wenn man das Browser Control nicht mal auf einem sichtbaren Fenster hat, sondern nur in einem versteckten Fenster hält. Auch in diesem Fall verliert das aktive Fenster den Fokus.

Aber mit einem kleinen Trick bekommt man es doch hin ( 🙂 warum sonst schreibe ich den Artikel )

  1. Man baut einen OnDocumentComplete Handler ein.
  2. Wenn das Event eintritt, besorgt man sich mit GetFocus das Fenster, dass aktuell noch den Fokus inne hat.
  3. Nun sendet man mit PostMessage eine selbst definierte Nachricht (#define WM_RESTOREFOCUS (WM_APP+x)) an den Container des Webbrowser Controls und übergibt als wParam einfach das Handle des Fensters, dass man soeben mit GetFocus ermittelt hat.
  4. Nach diesem Event wird der Skript-Code ausgeführt, der den Fokus stiehlt. Das stört uns nicht.
  5. Irgendwann kommt die Messageloop jetzt wieder an die Reihe und zieht die benutzerdefinierte Nachricht WM_RESTOREFOCUS aus der Queue.
  6. Man hat natürlich einen Handler für diese Nachricht im Container des Webbrowser Controls. Dieser macht nun nichts anderes als einen SetFocus auf das HWND Handle auszuführen, das im wParam übergeben wurde. Ein Test zuvor mit IsWindow versteht sich von selbst.

Dadurch, dass die Nachricht in der Message-Queue gepostet wird, wird sie zeitnah ausgeführt sobald wirklich der User wieder eine Chance selbst Eingaben zu machen. Problem zufriedenstellend gelöst.

Das sollte sich sogar mit C# oder VB hinbekommen lassen 😉