647998 - Should assert when a not-yet-fired nsITimer is GCed

Status: NEW

(Core :: XPCOM, enhancement)

Description

[Jesse Ruderman]

See bug 640629 for the latest round of screw-ups.

I understand that we can't change the GC behavior of nsITimer right now. Can we at least make it assert-fail, so it doesn't just make pieces of the browser fail quietly and mysteriously?  At least for one-shot timers?

Comment 1

[Benjamin Smedberg]

I've never understood why we can't make timers root themselves while they are pending (and we're not shutting down)... can we just do that, and perhaps deal with the little bit of leak fallout that might happen?

Comment 2

[Boris Zbarsky [:bzbarsky]]

That seems fine offhand to me.  Brendan, any reason not to do that?

Comment 3

[Jesse Ruderman]

Hrm, bug 387341 seems related.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I agree that the only sane model for timers is that they are rooted as long as they can fire in the future. It's terribly wrong for whether a timer fires or not to depend on the timing of GC.

We would have to audit users of repeating timers, at least, to make sure something cancels them. And obviously JS setTimeout/setInterval would have to cancel automatically when a window needs to go away.

Comment 5

[Boris Zbarsky [:bzbarsky]]

That last part works already: nsGlobalWindow::SetDocshell calls FreeInnerObjects on all the inners when aDocShell is null, and FreeInnerObjects calls ClearAllTimeouts, which cancels all the timer.  This covers the "window closing" case.  FreeInnerObjects is also called from nsGlobalWindow::SetNewDocument, which is what happens in the "navigating away" case.