1156094 - Remove the fictious requirement on the callers to nsITimer::InitWith* having to AddRef the timer

Status: RESOLVED WONTFIX

(Core :: XPCOM, defect)

Description

[(no longer active)]

The timer thread will hold a strong reference to the nsTimerImpl
object, so this requirement is unneeded.

Comment 1

[(no longer active)]

Attachment: Remove the fictious requirement on the callers to nsITimer::InitWith* having to AddRef the timer

Comment 2

[Nathan Froyd [:froydnj]]

I didn't look at this today because I wasn't mentally prepared to think about the correctness of timer code.  Still not prepared to think about this, but I did remember this bit of code, which suggests that the timer thread's reference is not sufficient:

http://mxr.mozilla.org/mozilla-central/source/xpcom/threads/nsTimerImpl.cpp#237

I'll try to look at this more closely tomorrow, but WDYT about that comment?

Comment 3

[(no longer active)]

Hmm, I don't know.  All I have to say is that most of our timers created from C++ don't have this extra ref, so the comment has to be inaccurate somehow.  :-)

Comment 4

[Nathan Froyd [:froydnj]]

I'd wager that most of our C++ callers are holding an extra ref as a nsCOMPtr member variable or similar...

Comment 5

[(no longer active)]

That's possible.  I admit I haven't audited them all...

Comment 6

[Nathan Froyd [:froydnj]]

Comment on attachment 8594513 [details] [diff] [review]
Remove the fictious requirement on the callers to nsITimer::InitWith* having to AddRef the timer

Review of attachment 8594513 [details] [diff] [review]:
-----------------------------------------------------------------

I'm going to cancel this review, because I'm having a hard time believing that removing that requirement from the docs is safe.  And I'm fairly sure that many many other uses of timers *do* hold the required ref, and things would start breaking badly if we removed those refs as well.

The nr_timer.cpp change seems reasonable from a "use smart pointer classes, don't manually call AddRef/Release" point of view, but please get a media/mtransport peer to review that.

The nsBindingManager.cpp change is just sad-making; I'm not sure if you can reasonably use smart pointers there or not.

Comment 7

[(no longer active)]

OK.  Let's call this WONTFIX then.