This is either a bug in global history, or in the one-shot timer implementation (or both). http://lxr.mozilla.org/seamonkey/source/xpfe/components/history/src/nsGlobalHistory.cpp#1710 Global history tries to fire a "syncTimer" after 10 seconds when history becomes dirty. This works fine for the simple case. However, if we're already dirty, and a one-shot timer is pending, then it cancels that timer and tries to reschedule the sync for another 10 second delay. Despite the NS_OK rv return from the call to InitWithFuncCallback, the syncTimer does not fire, and from that point on, the global history will never be flushed due to the syncTimer. This means that the only situations where we flush history to disk are (a) on the close of a window, and (b) when we shutdown. But given tabbed browsing, and the advent of 'popup blocking', a user might never open and close a new window during a browsing session. And if they crash at shutdown, the history for the session is lost. (I'm assuming that most users will eventually open two different URLs within 10 seconds at some point in a browsing session). http://lxr.mozilla.org/seamonkey/source/xpcom/threads/nsITimer.idl#152 Now, there is a comment in nsITimer.idl that "sort of" suggests that you can't really reuse a cancelled one-shot timer, although the comment isn't really correct since you can in fact cancel a one-shot timer. (You just can't re-initialize it and have it work). "Cancellation of timers applies to repeating timers " " (i.e., init()ed with aType=TYPE_REPEATING*). " The history bug can be fixed with this patch, although it seems sort of sucky that you have to destroy and re-create the timer just to reset the timeout. So, is this a bug in history, in timers, or both (and are there any other cases in the tree where it has been assumed that you can re-init a one-shot timer). Index: history/src/nsGlobalHistory.cpp =================================================================== RCS file: /cvsroot/mozilla/xpfe/components/history/src/nsGlobalHistory.cpp,v retrieving revision 1.163 diff -u -r1.163 nsGlobalHistory.cpp --- history/src/nsGlobalHistory.cpp 22 Nov 2002 07:55:42 -0000 1.163 +++ history/src/nsGlobalHistory.cpp 26 Nov 2002 06:19:45 -0000 @@ -1716,8 +1716,10 @@ { nsresult rv; - if (mSyncTimer) + if (mSyncTimer) { mSyncTimer->Cancel(); + mSyncTimer = nsnull; // can't re-init a one-shot timer ??? + } if (!mSyncTimer) { mSyncTimer = do_CreateInstance("@mozilla.org/timer;1", &rv);

So nice that pavlov doesn't read bugmail anymore...

you can't reuse timers... they hold state when they're fired and while they're firing. it should probably assert if you try to call Init more than once.

blaker, what happens when you Cancel but don't null mSyncTimer, and step into the second call to InitWithFuncCallback? Just from reading the code, I don't see what is going wrong -- but I haven't looked too hard. pavlov, it seems to me that re-init after cancel could work, and maybe should. It doesn't have to, but if it's easy, why not? /be

nsTimerImpl::Cancel() will set mCanceled to PR_TRUE. The init "sort of" succeeds, but when nsTimerImpl::Fire() is called, it bails immediately if mCanceled is true.

I also fixed a bug left over from the nsIScriptableTimer fiasco, where Init was calling SetDelayInternal with mType uninitialized. Comments? Blake, does this fix things for you? /be

I tried brendan's patch with a current trunk build, and this works fine for the history case. A cancelled timer can be successfully re-initialized and fired, and the syncTimer will reliably fire and flush history to disk. I also modified nsXULTooltipListener.cpp to re-use the tooltip timer. We were creating and destroying literally thousands of timers as you moved the mouse over content, and menus, trees, etc. in the UI. This patch works fine, and we could probably consider looking for other code that is repetitively creating and destroying one-shot timers (unless there is some yet undiscovered problem with the patch). (By the way, brendan. You said something about also fixing "SetDelayInternal with mType uninitialized" in your patch, but I don't see where that is.) Index: src/nsXULTooltipListener.cpp =================================================================== RCS file: /cvsroot/mozilla/layout/xul/base/src/nsXULTooltipListener.cpp,v retrieving revision 1.18 diff -u -r1.18 nsXULTooltipListener.cpp --- src/nsXULTooltipListener.cpp 19 Nov 2002 05:06:35 -0000 1.18 +++ src/nsXULTooltipListener.cpp 27 Nov 2002 02:54:59 -0000 @@ -112,7 +112,6 @@ // show the tooltip if we move the mouse out of the window if (mTooltipTimer && !mCurrentTooltip) { mTooltipTimer->Cancel(); - mTooltipTimer = nsnull; return NS_OK; } @@ -186,7 +185,11 @@ // go away only if it times out or leaves the target node. If nothing is // showing, though, we have to do the work. if (!mCurrentTooltip) { - mTooltipTimer = do_CreateInstance("@mozilla.org/timer;1"); + //static PRInt32 myCount = 0; + //printf("%p: nsXULTooltipListener::MouseMove, mTooltipTimer: %p, %d\n", + // this, mTooltipTimer, ++myCount); + if (!mTooltipTimer) // re-use, if available + mTooltipTimer = do_CreateInstance("@mozilla.org/timer;1"); if (mTooltipTimer) { nsCOMPtr<nsIDOMEventTarget> eventTarget; aMouseEvent->GetTarget(getter_AddRefs(eventTarget)); @@ -634,7 +637,6 @@ { if (mTooltipTimer) { mTooltipTimer->Cancel(); - mTooltipTimer = nsnull; mTargetNode = nsnull; } }

jrgm, how about a properly attached patch for that fix? Great work there, btw. (My comment about Init calling SetDelayInternal before mType was initialized referred to the diff at @@ -250,14 +250,14 @@.) /be

> @@ -250,14 +250,14 @@. Oh, right. Duh.

good god! this might explain why we're leaking memory-recycling timers...

Comment on attachment 107546 [details] [diff] [review] let the tooltip reuse the timer if it already has one sr=brendan@mozilla.org -- bryner, can you r= so we can get this into 1.3alpha? Thanks, /be

Comment on attachment 107534 [details] [diff] [review] Let a canceled timer be re-initialized I want this for 1.3alpha. If it's not sufficient, it seems to me there's a bug elsewhere, and we need to smoke it out with massive testing. But I think this is correct, and jrgm backs that (so far). /be

Comment on attachment 107534 [details] [diff] [review] Let a canceled timer be re-initialized sr=alecf (banking on a r=pavlov of course)

Comment on attachment 107534 [details] [diff] [review] Let a canceled timer be re-initialized You can't just set mCanceled = FALSE. The problem is is that if a timer is already alive as a PLEvent, then when it comes along to fire it will check to see if mCanceled is true or not. If it is true, then Fire() will not do anything, and release the last reference to the timer. Since you can't remove the PLEVent from the PLEventQueue, we'd have to have a list to keep track of which init a cancel is related to and which fire it is. Its a huge pain in the ass. We should just assert in Init() if mmCanceled is true.

From jrgm's measurements, this is worth fixing. Just waving the mouse was creating tons of one-shot timers, when one could be reused. The DOM level 0 setInterval code could also use this fix, I think. I'll try for a better patch later today. /be

I picked some style nits and made an InitCommon method to share the code that makes it safe to cancel and re-init. /be

Just fixed a few more places in handleTimerEvent that can use the new |timer| local variable, instead of its unchanging value, NS_STATIC_CAST(nsTimerImpl*, event->owner). /be

Pav's reviewing (I trust; he started to when we last spoke over IRC); alecf, care to do the honors again? jrgm, any more places where we should conserve timers? /be

I realized that there was a second usage in that file that could be converted to re-initing the timer (mAutoHideTimer), although that timer is only created when you stop moving the mouse (as opposed to on every mousemove for mTooltipTimer). I also added the initialization for the two member timers into the constructor. (By the way, just out of curiosity, the other member variable is 'nsAutoString mLastTreeCol'; I thought that (in general) nsAutoString's shouldn't be used as members [according to the alecf's string guide]). (Also, to some degree, 'KillTooltipTimer', and 'CreateAutoHideTimer' are a bit misnamed now (more like 'CancelTooltipTimer' and 'InitAutoHideTimer')). There are quite a few other places that, at a quick glance, would seem to be candidates for recycling a member timer. Perhaps, though, it would be best to land the timer changes and the tooltip changes; that should be enough to uncover any unexpected problems. Then, if it's looks good, I will file bugs to fixup at least the heavy users of transient one-shot timers.

Comment on attachment 107835 [details] [diff] [review] v2: let the tooltip listener reuse the tooltip and autohide timers if already available (or r=bryner, if hewitt's not around.)

::InitCommon must do an implicit Cancel, if necessary. Otherwise a timer could end up in two slots on the timer thread's list (implemented as an array). /be

Comment on attachment 107835 [details] [diff] [review] v2: let the tooltip listener reuse the tooltip and autohide timers if already available r=bryner

Comment on attachment 107835 [details] [diff] [review] v2: let the tooltip listener reuse the tooltip and autohide timers if already available sr=brendan@mozilla.org. Where's pavlov? I need his r= for my xpcom/threads patch, for 1.3alpha. /be

Comment on attachment 107845 [details] [diff] [review] all-in-one patch, with a fix to nsTimerImpl::InitCommon i think this patch will work right. lets give it a go :)

Comment on attachment 107845 [details] [diff] [review] all-in-one patch, with a fix to nsTimerImpl::InitCommon sr=alecf then.

I checked in attachment 107845 [details] [diff] [review]. Giving this to jrgm to close when he checks in. /be

checked in the tooltip listener patch. Marking fixed.

"I also modified nsXULTooltipListener.cpp to re-use the tooltip timer. We were creating and destroying literally thousands of timers as you moved the mouse over content, and menus, trees, etc. in the UI." I wonder if all of these timers being created and destroyed sometimes caused the situation where a tooltip doesn't disappear when the app loses focus. A long shot, sure, but it would be a nice side-effect!

Dean: yeah, I wondered that too. After running with these changes in my tree for a few days, I think I did see that happen once, so I don't think it's gone. (Possibly better?)

Comment on attachment 107829 [details] [diff] [review] proposed fix to let a canceled timer be re-initialized, take 3 hmm.. bugzilla shouldn't be putting obsolete patches in my request queue!

> hmm.. bugzilla shouldn't be putting obsolete patches in my request queue! Alec: bug 180652

The nsXULTooltipListener patch seems to have caused bug 184363.

reopening. There's anecdotal evidence that somehow the xultooltiptimer changes are causing a crash on OS/X [and later on Linux and win32 after some other tooltip related change]. I'll back out my change to nsXULTooltipListener.cpp for the purposes of 1.3b although eventually should figure out why this change is crashing at all.

looking for r+sr=jag and then approval for 1.3b

Comment on attachment 113460 [details] [diff] [review] patch to back out rev 1.19 to nsXULTooltipListener.cpp r=/sr=jag

backed out

adt: nsbeta1-