291561 - Freeze nsITimer or provide alternative

Status: RESOLVED WONTFIX

(Core :: XPCOM, defect)

Description

[Darin Fisher]

It would be good to expose timers in the frozen SDK.  Application developers and
extension authors using C++ could probably benefit from this.  I recall
discussions back at AOL about freezing this for a certain embedding customer,
but I couldn't find any related bugs.  The interface looks pretty well defined.
 Modulo some implementation issues (bug 291386 and bug 198524), I think the
interface is pretty reasonable.  Thoughts?

Comment 1

[Brendan Eich [:brendan]]

I object.  The fact that timers are an object shared among threads makes for
thread-safety pain in the current implementation.  Please consider an
alternative approach where a timer service hands out uint64 tokens.  The service
needs to be thread-safe, but clients don't have their hot little ref-counting
hands on the actual internal timer structs.

/be

Comment 2

[Brendan Eich [:brendan]]

I was just going through my bug list and noticed bug 198524.  Re-reading that
and bug 198517 just reinforced to me that we want integer-identified timers that
go away only when fired or canceled.

I changed my mind about uint32 being enough bits based on 2^32/(1000*3600*24) or
49 days being not so long that an XPCOM-based app might want greater or equal
uptime, yet face the problem of a very long-lived timer becoming aliased by a
newborn timer due to a timer being created every ms.

Now, creating an XPCOM timer every millisecond may seem totally insane, but it's
certainly doable on modern systems.  So uint64 is cheap enough and exponentially
more than enough that we should not have to worry about wraparound.

/be

Comment 3

[Benjamin Smedberg]

Why not keep the COM object, but have the object hold itself in existence until
the timer fires, instead of forcing the client code to hold it?

I don't understand the int-keyed timer idea. Does the timer still hold the
nsIObserver callback object?

I'm against freezing this iface the way it is... the combination of noscript C
callbacks and two different COM callbacks seems like overkill and will breed
permanent confusion.

Comment 4

[Brendan Eich [:brendan]]

(In reply to comment #3)
> Why not keep the COM object, but have the object hold itself in existence
> until the timer fires, instead of forcing the client code to hold it?

That's desirable, I think everyone agrees.

> I don't understand the int-keyed timer idea. Does the timer still hold the
> nsIObserver callback object?

Yes.  The int-keyed idea is not about changing timer members from strong to weak
refs.  It's about avoiding ugly race conditions.  See
http://lxr.mozilla.org/mozilla/source/xpcom/threads/nsTimerImpl.cpp#102.

> I'm against freezing this iface the way it is... the combination of noscript C
> callbacks and two different COM callbacks seems like overkill and will breed
> permanent confusion.

Another good reason.

We need a draft of a new interface that doesn't have the issues raised here. 
Who has time to do it?

/be

Comment 5

[Darin Fisher]

Is there something about this interface that makes it so broken?  It hasn't
changed since 2002!  It sounds to me as though we just do not like the fact that
users have to hold a reference to the object, that we don't like the multitude
of callback methods, and other aesthetic details.

Is there really any problem that prevents this interface from being used in a
portable fashion from moz 1.4 to now and into the future?

This interface could be implemented on top of whatever timer mechanism is most
suitable for Gecko itself.  Application and Extension authors may appreciate a
better interface, but they'd probably just be happy with something _stable_ that
works too.  I don't have much of any time to work on this for Gecko 1.8 -- does
anyone? :-/

Comment 6

[Brendan Eich [:brendan]]

The fact that the user holds a reference is not an aesthetic detail.

1.  It makes for ugly race conditions (no matter whether the timer thread also
holds a ref) that complicate the code.  I suppose we've solved those with code
complexity, a sunk cost that we can live with.  But it would be more efficient
in code space, and probably a wash in cycles, for clients to use a uint64 id.

2.  It also currently means that you have to manually break cycles with an out
of band protocol, as in bug 198517.  This is a big semantic difference.  If we
fix that by making timers non-ephemeral (by making the timer thread hold a
strong ref), then we will have fun with leaks, I predict.

I think almost everyone agrees in principle that 2 should be fixed.  Darin, do
you agree?  I think 1 will make the code simpler and easier to deal with when
fixing 2, although of course it's more work.

/be

Comment 7

[Darin Fisher]

All asynchronous APIs potentially have these observer / observee reference cycle
problems.  They are usually best solved by having the observee release its
reference to the observer when the single-shot event happens, when the observee
is canceled, or when the system is shutdown.  That way the observer does not
have to be careful to break the cycle.  I don't see why that is not a solution here.

nsTimerImpl::Cancel should clear out mCallback; it doesn't.  The xpcom timer
subsystem should listen for xpcom-shutdown; does it?  I see
nsTimerImpl::Shutdown, but I haven't looked carefully to see if it does what it
should in regards to breaking these cycles.  I suspect it doesn't.

Comment 8

[Darin Fisher]

> All asynchronous APIs potentially have these...

Just to be clear, I'm talking XPCOM and reference counted memory management
here, and of course weak references and other tricks are used to avoid these
problems.  But, in many of the Necko APIs this very problem exists.  The
interfaces look a lot like nsITimer, they involve background threads, etc. 
There is nothing special about nsITimer.

Comment 9

[Darin Fisher]

> nsTimerImpl::Cancel should clear out mCallback; it doesn't.  The xpcom timer
> subsystem should listen for xpcom-shutdown; does it?  I see
> nsTimerImpl::Shutdown, but I haven't looked carefully to see if it does what it
> should in regards to breaking these cycles.  I suspect it doesn't.

So, nsTimerImpl::ReleaseCallback is only run from ~nsTimerImpl or when the timer
is re-initialized.  So, that is the problem.

So, do we really need to change the timer interface?  Or, is it sufficient to
just fix the bugs in its implementation?

Comment 10

[Brendan Eich [:brendan]]

(In reply to comment #9)

> So, do we really need to change the timer interface?  Or, is it sufficient to
> just fix the bugs in its implementation?

See bug 118004, where I did fix the ephemeral timer bug (sorry I forgot doing
that; it was forgettable).

We don't need to change the timer interface to fix a bunch of bugs, and in fact
bug 118004 comment 24 even talks about a thread-safe design that does not share
memory between clients and the timer thread, yet still has clients using XPCOM
timer objects.

Darin, you are right about the general problem of cycles with observers.  But
whether the client holds an XPCOM strong ref to an object shared with the timer
thread, or a weak uint64 token mapped to the object strongly referenced by the
timer thread, is a difference in implementation that's hard to hide behind the
current interface, unless we do exactly what dbradley and I discussed (again
allued to in bug 118004 comment 24; I think our discussion was over IRC).

If we do keep something like the current interface but decouple client object
from timer thread internal structure, mapping using some kind of token, then
we've preserved the current API (or something close to it) at the price of extra
objects.  At this point that's a good trade, no question.

But if we could have the token-based interface be the public, frozen one, then
we could migrate our current nsITimer client code toward that lower-overhead
approach, and new users of the frozen interface could avoid extra objects that
peer the ones in the timer thread's queue, from the get-go.

Does this make any sense yet?

/be

Comment 11

[Darin Fisher]

I'm with you totally now :-)

Comment 12

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

We don't freeze interfaces anymore.