Non-psychic debugging: Why you’re leaking timers


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

Comments (22)
  1. Anonymous says:

    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.

  2. Henning Makholm says:

    "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.

  3. Saveddijon says:

    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.)

  4. Chris Taylor says:

    "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.

  5. Chris Taylor says:

    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.

    [True, but there's another reason which is probably more likely… -Raymond]
  6. A Guy Somewhere Cold says:

    @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.

  7. Kujo says:

    The pragma doesn't care about type or scope, so referencing Foo::SetTimer() or enum { SetTimer } will be considered deprecated.

    [That's what I was looking for. A lot of classes have a method called SetTimer. -Raymond]
  8. Anonymous says:

    It's over 9000…!

  9. PhiSmi says:

    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.

  10. Timothy Byrd says:

    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.

  11. asf says:

    Why no IsWindow in the assert?

  12. DWalker says:

    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."

  13. DWalker says:

    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.

  14. Dave says:

    Code analysis tools can use this information to attempt to identify potential defects.

    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.

    assert(hWnd != NULL);

    … which will only work in the non-release version of the code, thus making it anything from "completely useless" through to "only marginally effective".

  15. Neil says:

    (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…)

  16. Mark Steward says:

    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.

  17. Gabe says:

    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."!

    [My guess (total guess) is that the "set up the F2 timer" function was called a second redundant time, at which point the handle was valid and the timer ran as expected. The only consequence was the leaked timer from the first call. -Raymond]
  18. Tergiver says:

    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.

  19. Gabe says:

    Tergiver: I can't think of any key action that triggers on WM_KEYUP instead of WM_KEYDOWN. Can you name some?

  20. Tergiver says:

    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).

  21. GregM says:

    Tergiver, it's only "wrong" if you don't want key repeat to trigger the action multiple times.

  22. Tergiver says:

    Which is what I said the first time: "trigger an action (which is subject to repeat)"

Comments are closed.

Skip to main content