Ticket #10052 (confirmed defect)

Opened 2 years ago

Last modified 7 days ago

bogus timer id assertion in wxTimerProc if timer stopped when it is about to fire

Reported by: jteh Owned by:
Priority: normal Milestone:
Component: wxMSW Version: 2.8.x
Keywords: wxTimerWndProc TimerMap KillTimer Cc: sobekjunk@…
Blocked By: Patch: no
Blocking:

Description

Under Windows, wx sometimes generates "bogus timer id in wxTimerProc" assertion messages if a timer is stopped just before it was about to fire. This is because the WM_TIMER message for the timer has already been posted to the queue, but the win32 KillTimer function (which is called by wxTimer::Stop) does not remove these messages from the queue if they have already been posted. Thus, when the WM_TIMER message for the stopped timer is processed, the timer does not exist anymore, causing the assertion to fail. This behaviour is described in  this thread from 2006.

While it is possible to disable assertion messages, this message is not particularly useful even when debugging, especially given this documented behaviour of the win32 KillTimer function. Either this message should be removed for Windows or a way needs to be found to remove the offending WM_TIMER messages from the queue.

My particular case of this issue actually occurs when using wxPython, where I need to disable assertion messages altogether to stop this from causing exceptions. This is obviously undesirable, as there may be other assertion messages that *are* important. My case is described in  this thread.

(I didn't file this under wxPython because i believe the issue to be with wxMSW, not wxPython.)

Change History

  Changed 2 years ago by vadz

  • status changed from new to confirmed

I'm still reluctant to remove this assert because it could catch real bugs too. Calling PeekMessage(..., WM_TIMER, WM_TIMER, PM_REMOVE) from Stop() should take care of it and IMO is worth doing. If you can do it and test that it does fix the problem, I'd be definitely glad to apply a patch doing this.

TIA!

  Changed 3 months ago by sobekjunk

  • cc sobekjunk@… added
  • keywords wxTimerWndProc TimerMap KillTimer added

I am having this problem in 2.8.11 and plan to modify my version to remove the assert. Since the documentation states in the SDK's remark section:
The KillTimer function does not remove WM_TIMER messages already posted to the message queue.

I would suggest removing the wxCHECK_MSG (i.e,. the assert) from wxTimerWndProc. I believe the rest of the code contains enough sanity checks to catch other bugs. Is there a case, other than the above, when you receive an invalid wParam? Should wx handle it gracefully or should it stop the program? I'm all for sanity checks but this is asserting on valid code.

This is all moot if you can modify Stop to remove all occurances of messages from the killed timer. However, is the code quicker if it just ignores invalid wParams (i.e., the iteration through the message queue for each stopped timer can add up)?

  Changed 3 months ago by vadz

So have you tried the PeekMessage() fix proposed above or not? I'd just like to know to avoid spending time on this myself if you already did...

follow-up: ↓ 5   Changed 3 months ago by sobekjunk

I did not try the PeekMessage method. It sounds like it should work but, for me, I would rather it gracefully ignore timers that no longer exist.

in reply to: ↑ 4   Changed 3 months ago by vadz

Replying to sobekjunk:

I did not try the PeekMessage method. It sounds like it should work but, for me, I would rather it gracefully ignore timers that no longer exist.

I feel like there is some misunderstanding here. Adding PeekMessage() should make it work exactly like this, i.e. ignore timers that no longer exist. Again, I didn't test it and unfortunately it seems nobody else is motivated to do it neither...

  Changed 3 months ago by sobekjunk

There is some misunderstanding. I didn't test the PeekMessage method since I wanted to avoid the constant iteration through all messages to remove any timer messages. It seems like unneeded overhead, especially if you have multiple timers with very small periods and a lot of user interaction. Is the PeekMessage solution the one you want to implement? Sorry for not fully explaining why I didn't do the PeekMessage implementation in the previous post.

follow-up: ↓ 11   Changed 3 months ago by vadz

I honestly didn't think at all about performance aspects of this. Do you really call Stop() (which would be doing this) often enough for it to be a problem?

  Changed 3 months ago by sobekjunk

Indirectly I call Stop by calling Start (i.e., Restart). I was experimenting (still am experimenting) with multiple methods of rendering. The method being tested was using a combination of listening to idle events and using a render timer. I did not RequestMore in the idle event processing to avoid the constant overhead / spinning that it results in. The end result is whoever renders resets the timer. This method increased the timer's lack of fidelity when trying to achieve quick render rates while still drawing during menu events which solely listening to idle events does not do.

The constant restarting of timers led to it crashing.

  Changed 3 months ago by sobekjunk

I forgot to mention in the above that I only use one shot timers.

Another render method is to use idle events and call RequestMore() and render when it is time to render and use a backup wxTimer. The backup timer acts as a fail-safe (i.e. a dead man's switch) so that I would always render at a minimum rate and it would also ensure that I would render when menu UI was occurring. During ideal circumstances the timer would always be reset since every idle event would restart the timer. If it did not receive an idle event (lots of events or menu UI), then the timer would finally reach zero and ensure rendering continues (one shot timer which would restart itself in the timer event). This method is a great solution to the idle event menu UI issue.

  Changed 3 months ago by aidandunne

In case this helps anybody:

We were experiencing what appeared to be this problem with some of our code, but then I noticied that the timer creation and initialization routine was being called directly from the wxApp constructor.
Since the wxTimerHiddenWindowModule::ms_hwnd handle was getting reset *after* wxTimer initializations, this meant that some intial timer events were targeting bad HWNDs.

I originally came to the same conclusion as detailed in this thread, based on my observation of the wxTimer::m_id re-generation code, but I couldn't reproduce it with a simple modification of one of the wxSamples.

Again this might not help anyone here as it might not be related to some core problem that we do not see, but might be useful in case others come across this thread who are starting timers from the wxApp constructor ( or before the TimerHiddenWindowModule::OnInit() is called at least).

The issue only arose after we tried out 2.8.11; does not happen with 2.8.8

cheers.

in reply to: ↑ 7   Changed 7 days ago by hajokirchhoff

Replying to vadz:

I honestly didn't think at all about performance aspects of this. Do you really call Stop() (which would be doing this) often enough for it to be a problem?

I experience the same problems, only with svn-head.

I too would vote for simply removing the assert. Also I don't know what 'real bugs' this assert might catch. Certainly not user bugs, as the user does not have access to the timer map.

My view is: if the WM_TIMER id is no longer in the timer map, the wxTimer must have been stopped or destroyed and the event can safely be discarded.

Regards, Hajo

  Changed 7 days ago by hajokirchhoff

I forgot to add: In my case the bogus assert comes at the end of my program, when the wxTimer is destroyed by a destructor. In about 1 out of 10 cases I get this assert.

Note: See TracTickets for help on using tickets.