472258 - Reinitializing one-shot timers by resetting delay (->SetDelay) doesn't work anymore - fix callers

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Nickolay_Ponomarev]

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.

Comment 1

[Nickolay_Ponomarev]

Attachment: patch with one such usage fixed

(this fixes bug 419292)

Comment 2

[Boris Zbarsky [:bzbarsky]]

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

Comment 3

[Nickolay_Ponomarev]

Attachment: patch [Checkin: Comment 8]

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.

Comment 4

[Boris Zbarsky [:bzbarsky]]

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

Comment 5

[Nickolay_Ponomarev]

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.

Comment 6

[Nickolay_Ponomarev]

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

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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 8

[Serge Gautherie (:sgautherie)]

Comment on attachment 355603 [details] [diff] [review]
patch
[Checkin: Comment 8]


http://hg.mozilla.org/mozilla-central/rev/cdd059f3d200