Closed Bug 472258 Opened 11 years ago Closed 11 years ago

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

Categories

(Core :: XPCOM, defect)

x86
All
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.2a1

People

(Reporter: asqueella, Assigned: asqueella)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

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.
Attached patch patch with one such usage fixed (obsolete) — Splinter Review
(this fixes bug 419292)
Blocks: 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.
Attachment #355519 - Attachment is obsolete: true
Attachment #355603 - Flags: superreview?(bzbarsky)
Attachment #355603 - Flags: review?(bzbarsky)
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.
Blocks: 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.
Attachment #355603 - Flags: superreview?(bzbarsky)
Attachment #355603 - Flags: superreview+
Attachment #355603 - Flags: review?(bzbarsky)
Attachment #355603 - Flags: review+
Status: NEW → ASSIGNED
Keywords: checkin-needed
Comment on attachment 355603 [details] [diff] [review]
patch
[Checkin: Comment 8]


http://hg.mozilla.org/mozilla-central/rev/cdd059f3d200
Attachment #355603 - Attachment description: patch → patch [Checkin: Comment 8]
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
You need to log in before you can comment on or make changes to this bug.