Due to the fixes for bug 407201 / bug 330128 (I think) one-shot timers' state is clobbered in nsTimerImpl::Fire: http://mxr.mozilla.org/mozilla-central/source/xpcom/threads/nsTimerImpl.cpp#408 This means that reinitializing timers by calling SetDelay doesn't work anymore: timer->Init(..., ONE_SHOT) // timer fired timer->SetDelay(100); /* the user expects the callbacks passed to Init() above to fire after the specified delay */ And the users must re-initialize the timer: timer->Init(..., ONE_SHOT) // timer fired timer->Init(..., ONE_SHOT) // to make it fire again Most uses of timer in the tree do this, except a few (the ones I found use InitWithFuncCallback). I think the remaining few should be fixed and an assertion should be added to SetDelay() to catch this incorrect way of re-initializing timers. If my understanding is incorrect and we should support re-initializing timers by resetting delay, please comment.

(this fixes bug 419292)

I suppose we could not drop function callbacks on one-shot timers to restore the old behavior. But I'd also be fine with making this API change (esp. because the old behavior was not documented to work, while the new one certainly was) and fixing the callers...

Same as above with proper change in nsWebShellWindow, changes to nsITimer docs and a comment explaining correct, but suspicious-looking code in xpconnect. Turns out other suspicious places are using timers correctly. I went through mozilla-central looking for InitWithFuncCallback(TYPE_ONE_SHOT) calls, checking if any of them re-initializes the timer via SetDelay() like the code in nsWebShellWindow. I added an assertion to SetDelay (and switched to a failure rv so that this error is more visible in non-debug builds). I think it's better if there's one way to reinitialize the timer and I didn't see other complaints, even though the original change went into 3.0.

Can nsWebShellWindow::SetPersistenceTimer fire while the timer is armed?

Yes, it happens all the time. I don't think this causes any problems wrt InitWithFuncCallback changes. On the other hand, your comment made me think about mSPTimerLock business, I think it should be removed -- separate bug.

> On the other hand, your comment made me think about mSPTimerLock business, I > think it should be removed -- separate bug. bug 472363.

If possible I'd prefer to stick with current behavior for timers. We have to for callback objects to avoid leaks, so it makes sense that callback functions behave the same.

Comment on attachment 355603 [details] [diff] [review] patch [Checkin: Comment 8] http://hg.mozilla.org/mozilla-central/rev/cdd059f3d200