I was not involved in this debugging puzzle,
but I was informed of its conclusions,
and I think it illustrates both the process of debugging
as well as uncovering a common type of defect.
I've written it up in the style of a post-mortem.
A user reported that if they press and hold the F2 key
for about a minute,
our program eventually stops working.
According to Task Manager, our User object count has reached
the 10,000 object limit,
and closer inspection revealed that we had created over 9000 timer
objects.
We ran the debugger and
set breakpoints on SetTimer and KillTimer
to print to the debugger each timer ID as it was created and destroyed.
Visual inspection of the output revealed that all but one of the IDs
being created was matched with an appropriate destruction.
We re-ran the scenario with a conditional breakpoint on
SetTimer set to fire when that bad ID was set.
It didn't take long for that breakpoint to fire,
and we discovered that we were setting the timer against a
NULL window handle.
A different developer on the team arrived at the same
conclusion by a different route.
Instead of watching timers being created and destroyed,
the developer dumped each timer message before it was dispatched
and observed that most of the
entries were associated with NULL window handles.
Two independent analyses came to the same conclusion:
We were creating a bunch of thread timers and not destroying them.
A closer inspection of the code revealed that
thread timers were not intended in the first place.
Each time the user presses F2,
the code calls SetTimer and passes a window handle
it believes to be non-NULL.
The timer is destroyed
in the window procedure's WM_TIMER handler, but
since the timer was registered against the wrong window handle,
the WM_TIMER is never received by the intended
target's window procedure,
and the timer is never destroyed.
The window handle is NULL due to a defect in the code
which handles the F2 keypress:
The handle that the code wanted to use
for the timer had not yet been set.
(It was set by a later step of F2 processing.)
The timer was being set by a helper function which is called both
before and after the code that sets the handle, but it obviously was
written on the assumption that it would only be called after.
To reduce the likelihood of this type of defect being introduced
in the future,
we're going to introduce a wrapper function around SetTimer
which asserts that the window handle is non-NULL before
calling SetTimer.
(In the rare case that we actually want a thread timer,
we'll have a second wrapper function called
SetThreadTimer.)
I haven't seen the wrapper function, but I suspect it goes
something like this:
inline UINT_PTR SetWindowTimer(
__in HWND hWnd, // NB - not optional
__in UINT_PTR nIDEvent,
__in UINT uElapse,
__in_opt TIMERPROC lpTimerFunc)
{
assert(hWnd != NULL);
return SetTimer(hWnd, nIDEvent, uElapse, lpTimerFunc);
}
inline UINT_PTR SetThreadTimer(
__in UINT uElapse,
__in_opt TIMERPROC lpTimerFunc)
{
return SetTimer(NULL, 0, uElapse, lpTimerFunc);
}
__declspec(deprecated)
WINUSERAPI
UINT_PTR
WINAPI
SetTimer(
__in_opt HWND hWnd,
__in UINT_PTR nIDEvent,
__in UINT uElapse,
__in_opt TIMERPROC lpTimerFunc);
There are few interesting things here.
First, observe that the annotation for the first parameter to
SetWindowTimer is __in rather than
__in_opt. This indicates that the parameter
cannot be NULL.
Code analysis tools can use this information to attempt to identify
potential defects.
Second, observe that the SetThreadTimer wrapper
function omits the first two parameters.
For thread timers, the hWnd passed to
SetTimer is always NULL and the
nIDEvent is ignored.
Third, after the two wrapper functions, we redeclare
the SetTimer, but mark it as deprecated
so the compiler will complain if somebody tries to call the
original function instead of one of the two wrappers.
(The __declspec(deprecated) extended attribute
is a nonstandard Microsoft extension.)
Exercise:
Why did I use __declspec(deprecated) instead of
#pragma deprecated(SetTimer)?
The API they ended with is what the API should have been since the beginning – no magic values selecting a different magic meaning, and each function doing just one thing.
"Why did I use __declspec(deprecated) instead of #pragma deprecated(SetTimer)?"
Not being psychic, I can only guess: It must have something to do with the fact that __declspec is much better suited for preprocessor tricks that you might need in order to push the code through different compilers, analysis tools, and the like.
What I'd like to know: given the application in question, would it be reasonable for a user to hold down the F2 key for an entire minute? How was this even discovered? (Perhaps it is reasonable; I don't know what this application is, or what it does. If it's a racing car game where F2 maps to the gas pedal then maybe it's very reasonable to have it pressed for a full minute.)
"Why did I use __declspec(deprecated) instead of #pragma deprecated(SetTimer)"
I prefer the using __declspec(deprecated) because you have the option to provide a message to the developer informing them of the alternate API functions. But it does not seem that you are taking advantage of that in this case.
Ah, it just came to me. Using the pragma would mark all overloads deprecated which might be a problem if other libraries or utility functions provide a function with the same name but different signature.
@Saveddjion: Thorough application testing means not only testing the expected code paths, but the unexpected ones. Users can do some really bizarre things for reasons you might never have expected, and I think it would be rather arrogant to suggest that a given bug is a user's fault because they were using the application wrong.
The pragma doesn't care about type or scope, so referencing Foo::SetTimer() or enum { SetTimer } will be considered deprecated.
It's over 9000…!
The F2 for a whole minute is not very relevant. Every F2 press/repeat is going to leak a timer resource. It just took a minute to exhaust them all. Don't get distracted by how the problem was discovered; the problem is valid and ought to be resolved. If the application ran for a long time the F2 will eventually be pressed enough times.
PhiSmi's answer is better, but I was going to say that having the corner of a book or notebook accidentally hold a key down is not unheard of in my experience.
Why no IsWindow in the assert?
Don't press and hold the F2 key for a whole minute! (Although it might be appropriate, in the context of the program.)
Doctor, it hurts when I do *this*. "Then, don't do that."
Ah, yes, a book on the keyboard could certainly cause that problem. Or a cat, which happens at my house often enough. Once, our cat bought an expensive piece of art from EBay. My partner learned not to leave the screen up with the "buy now" button visible and leave the computer.
Unfortunately PREfast's ability to detect use of NULL pointers isn't very good, and conversly it produces large numbers of false positives for these, so I wouldn't rely on this.
… which will only work in the non-release version of the code, thus making it anything from "completely useless" through to "only marginally effective".
(In the rare case that we actually want a thread timer, we'll have a second wrapper function called SetThreadTimer.)
SetThreadTimer will still need the nIDEvent parameter in case you need to reset an existing timer. (Or create a ResetThreadTimer wrapper…)
Dave: why on earth would you turn a resource leak into a fatal error on a release product? If passing a null value will cause serious problems during normal usage, don't use an assert.
asf: IsWindow is not thread safe, and you're more likely to pass a valid (but wrong) handle than a non-existent one. At best, you'd catch a corrupt variable just before SetTimer does.
While it's certainly an interesting question as to how they discovered that holding down F2 for a minute makes the app stop working , you really have to ask why nobody noticed that the timer wasn't working! I'm sure the test plan is much more likely to include "Press F2. Verify that X happens after Y seconds." than "Hold down F2 for 1 minute. Verify app still works."!
The other flaw that is missed in the analysis is that the developer used the WM_KEYDOWN event to trigger an action (which is subject to repeat), rather than WM_KEYUP which is the 'more correct' choice. Mouse/Key down is for predicate to an action (e.g. selection), up is where actions should trigger.
Tergiver: I can't think of any key action that triggers on WM_KEYUP instead of WM_KEYDOWN. Can you name some?
Frankly, I think most programmers simply get it wrong. The only case that comes easily to mind where it is correct is Shift+F10 (or the context menu key). Of course this only works correctly if the programmer responds to WM_CONTEXTMENU instead of WM_RBUTTONUP (or if you're really green you think it should be WM_RBUTTONDOWN). WM_CONTEXTMENU is fired on WM_KEYUP (I believe TranslateMessage does the job of sending WM_CONTEXTMENU).
Tergiver, it's only "wrong" if you don't want key repeat to trigger the action multiple times.
Which is what I said the first time: "trigger an action (which is subject to repeat)"