In recent trunk mozilla builds, I'm seeing cases where during editing the caret will either not show up, or it stops blinking. Others on IRC have noticed that too. It seems random, so I haven't come up with reproduceable steps yet.

Leon, could this be due to the changes you made to the caret to reuse the same timer?

ok, I will try.

the recent change to re-init timer is in bug 207469 and bug 204005.

I think this is related to bug 207469,because when crate disappear, I can find ASSERTION below: ###!!! ASSERTION: Hey, this ain't my timer!: 'timer == mUpdateTimer.get()', file ./nsComposerCommandsUpdater.cpp, line 384 in codes of function: nsresult nsComposerCommandsUpdater::Notify(nsITimer *timer) { NS_ASSERTION(timer == mUpdateTimer.get(), "Hey, this ain't my timer!"); mUpdateTimer = NULL; // release my hold TimerCallback(); return NS_OK; } Because we have reinit the mUpdateTimer, so we should not release it here.

just removing "mUpdateTimer = NULL; // release my hold " dose not work. so go back to orginal status. I want to discard my changed to those codes. .....

orginal patches can only decrease call timer process about 0.5% in sol8, but caused this regression, so discard it and go back to orginal status.

I test it , and current caret is ok.

typo: orginal patches can only increase call timer process about 0.5% in sol8, but caused this regression, so discard it and go back to orginal status.

kin: btw, I have give patch which clean cache for caret pos after turn off cache, and momoi and has tested it, looks ok and can reslove Japanese and Chinese input issues. further more, cleaning actions is still necessary to provide a fresh cache for further usage.

Whoa, how is reinitting a timer in composer affecting the totally independent caret timer? Do we have a bug with timers here? If so, we need to fix that, not band-aid.

Removing the line mUpdateTimer = NULL; // release my hold should have made things better, or no worse. It seems like a needed correction, even if there are other bugs to diagnose and fix. With this line removed, what still goes wrong? Can you debug to find out more about why it goes wrong (whatever "it" is)? /be

As a sidenote, I have had a feeling that there is a bug in timers, although I'm not sure if it's related. Sometimes, quite rarely, some timer-using functions in the ui just stop working (throbber, url-bar history, actual navigation, tooltips etc). I haven't seen this problem for a little while though.

Might be related to something else happening at the same time. My caret just stopped blinking while I was composing a message and biff fired and downloaded one new message.

I just noticed that the event state manager's mBrowseWithCaret is true when running in Composer! So this may also be contributing to the caret's disappearance when the timer dies ... that is if I disable the PrimeTimer() call in nsCaret so that the caret never blinks, and I click in the content area, the caret is drawn as usual during placement of the caret in the new location, but then the browse with caret code in esm is triggered which draws/inverts the caret so it gets erased.

... of course I forgot to mention that you'll only see this problem in composer if you've enabled caret browsing in the browser.

Leon have you made any progress debugging the timer problem that sfraser and brendan mention above?

In orginal codes, mUpdateTimer was released and recreated, then set callback fucntion in nsComposerCommandsUpdater::PrimeUpdateTimer. when process timer in timerthread, nsComposerCommandsUpdater::Notify will be called, and it set null to mUpdateTimer and call callback function. In current codes, we have wanted to reuse the mUpdateTimer, and do not release it in nsComposerCommandsUpdater::PrimeUpdateTimer. but when call nsComposerCommandsUpdater::Notify, mUpdateTimer have been set null. so in next cycle calling nsComposerCommandsUpdater::PrimeUpdateTimer, we still recreate the mUpdateTimer in fact. Now I think in fact there should exist "mUpdateTimer->Cancel();" before "mUpdateTimer = NULL; " in nsComposerCommandsUpdater::Notify. (there exist same codes in orginal nsComposerCommandsUpdater::PrimeUpdateTimer which can remove timer from timerthread) About current codes, I still think removing "mUpdateTimer = NULL; // release my hold" can resolve this bug. I tested today, looks ok(I feel suprised about previous results days before).

editorbase+ -> leon.zhang

Comment on attachment 125734 [details] [diff] [review] proposed fix Can this bug be reproduced? Anyway, this patch is helpful according to comments of Brendan.

Comment on attachment 125734 [details] [diff] [review] proposed fix no assertion, if this patch is applied.

more explanations following comment #17: TimerManager of AppShell controll its mIdleTimers, which include mUpdateTimer of nsComposerCommandsUpdater. In current codes of nsComposerCommandsUpdater.cpp: After call nsComposerCommandsUpdater::Notify at first time, mUpdateTimer is set null and do NOT be removed from mIdleTimers. Then create a new mUpdateTimer in nsComposerCommandsUpdater::PrimeUpdateTimer. In the next cycle of calling nsComposerCommandsUpdater::Notify fired from the old mUpdateTimer, the ASSERTION will appear because of difference of the new and old mUpdateTimer. In a word, to release a timer, we should not just set null to it jusk like current codes, but call timer->Cancel().

Comment on attachment 125734 [details] [diff] [review] proposed fix since we have reinited the mUpdateTimer, so we should remove this line: "mUpdateTimer = NULL; // release my hold ". I am not sure the old test result in comment #5, maybe that code tree is not very clean. I test this patch, and analyzed the whole debug debug log related to timer, and do not find any incorrectness.

Comment on attachment 125734 [details] [diff] [review] proposed fix I'm ok with this fix, since it's necessary if you want to re-use the same timer, but what I, and I think others, are more iterested in are the possible timer bugs this is exposing. What I don't get is the fact that on IRC you described a situation where it sounded like the old timer was being fired twice, with a PrimeUpdateTimer() call in between the 2 firings? Shouldn't this old one shot timer only fire once? Perhaps, we should also be calling mUpdateTimer->Cancel() in PrimeUpdateTimer() if mUpdateTimer already exists to make sure it gets removed from all queues? Or make sure that the re-init'ing of an existing timer does a cancel? On top of that, there is still the question of why this composer timer affects the caret blink timer which is totaly separate? Have you done any poking around in timer code to see why these issues are occuring?

So sfraser and I sat down together and poked around in the timer code. Current theory for the one shot timer problem is this: 1. When it's time for the old timer to fire, TimerThread::Run(): A. Removes the old timer from the mTimers queue. B. Sets the old timer's mArmed field to false. C. Posts a PLEvent which will call handleTimerEvent() on the old timer. 2. When the PLEvent gets handled, the timer gets added to the TimerManager's mIdleTimer queue instead of firing right away. 3. nsComposerCommandsUpdater::PrimeUpdateTimer() is triggered before the old timer gets fired off from the mIdleTimer queue, and attempts to re-init the old timer. This allows the old timer to be placed on the timer thread's mTimers queue again. 4. The old timer gets fired off from the mIdleTimer queue, which nulls out the mUpdateTimer in the nsComposercommandsUpdater. The whole process then repeats, except that when we hit step #3 again, a new timer is created because of the previous firing of the old timer. We then hit the assertion. Calling timer->Cancel from nsComposercommandsUpdater::PrimeUpdateTimer() won't help us right now because it currently does not remove any instances of the timer on the mIdleTimer queue. I think it should be modified so that it does. ---- As for how this relates to the caret blink timer, I'm guessing that there may be some interaction with the paint updates generated during the firing of the ComposerCommandsUpdater timer. When we paint, the presShell has some logic that automatically turns the caret blinking off, before painting, and then turns it back on when it's done. Perhaps there is some strange issue with re-init'ing repeating timers when the timer already has instances of itself in the mIdleTimer queue?

Gahhh. Pavlov, idle timers do not mix with re-init'ing an existing timer to save on create-instance costs. At the least, shouldn't mIdle being true cause the re-init code to do something special (force the timer to fire, or make a new timer instance to replace the one referred to by|this| in the mIdleTimers array? /be

Comment on attachment 126421 [details] an old log file I printed(see line: 21,83,1439,1442,1459,1794) This log include main parts below: 1) create mBlinkTimer(line 22) and the first mUpdateTimer(line 83). when call InitWithFuncCallback, each will call TimerThread::AddTimer and inserted them into mTimers of TimerThread. 2) when TimerThread::Run, timers will be removed from mtimers of timerthread and create a new TimerEvent (timer->PostTimerEvent();) . then this timer will be added into TimerManager's mIdleTimers when run handleTimerEvent. 3) when nsAppShell::Run, nsTimerManager::FireNextIdleTime will fire timers in TimerManager's mIdleTimers, then remove the timer. About source of this assertion: In current codes, we have been reusing old mUpdateTimer(call InitWithFuncCallback again and again) till we call nsComposerComandsUpdater::Notify first time(line 1436). *** Note: Before nsComposerComandsUpdater::Notify, we have called ::PrimeUpdateTimer (line 1426)which insert the old timer into mtimers of timerthread) Following nsComposerComandsUpdater::Notify, TimerThread::Run() will remove this old timer from mtimers and insert it into idletimers of timermanager. Then create a new timer in next calling PrimeUpdateTimer(becuase we set null to timer in Notify). So,In following nsTimerManager::FireNextIdleTime, old timer be fired again!!!(line 1793), then cause this assertion!!! About the interaction between mBlinkTimer and mUpdateTimer: I am not sure that there exist interaction between them, especaily after applied this patch. I can only see this assertion now without caret disappear or stopping blink.

remove unnecessary and redudent info, then replace adress of timer with a name, you can get the log pasted above

Comment on attachment 126438 [details] another running log see line 224: Although we have set null to mUpdateTimer in Notify, because the ref to the timer have not been removed from mtimers of timerthread, so if timerthread::Run here, the timer was appended into midletimers of timermanager. next cycle of calling Notify will fire assertion!!!

Timerthread is an independent thread, and it can be schduled in some cases. in current codes: nsComposerCommandsUpdater::Notify -> nsComposerCommandsUpdater::TimerCallback() ->UpdateCommandGroup(NS_LITERAL_STRING("style")). I remember UpdateCommandGroup can lead to the siwtch of thread!! (I have fixed a bug 1.2.1+gtk2 which also modify menu and lead to process another event without finishing current event) If TimerThread::Run is scheduled in nsComposerCommandsUpdater::TimerCallback()(in fact it happened in the second log file!! line: 224), this will lead to timerthread process a unremoved but useless timer, further more timermanager will process it again and produce assertion info.

we should ensure the correctness of usage timer at first. As for the interaction of mBlinkTimer and mUpdateTimer, I think if the timerthread and timermanager can manage each timer correctly, should not produce incorrectness. I still can not find caret diappear or stop blink till now, but source of assertion should be correct.

I can reproduce this bug (caret disappear or stop blink ) now in an old trunk + related patches. I will give analysis related to caret Blinktimer soon.

I can repdoduce this bug in a pc(AMD1800plus + 512MDDR333 + oldertrunk + reinit timer patches) In fact, the caret did not disappear or stop blinking. Caret has been drew two times each time, so it looks like caret disappear/stop blinking. I print the debug info related to timer process, and found that the timer is still effetive and is fired at intervals of stable preriod. But, when bug appear, I can find the timer is be fired 2 times(or there exist 2 same timers in mIdleTimers). This can lead to drawing caret 2 times, so the caret disppear or stop blink. As for the reason of this bug, we have reused the mBlinkTimer which can be appended to mtimers of timerthread or mIdleTimers of timermanager many times, further more it can be fired many times. If we can ensure there exist just one element refered to the timer, we can prevent this bug. I verified this solution after remove the timer from timers array before insert.

Ok, here's an alternate to leon's patch. What it does, is it modifies Cancel() and InitCommon() in the timer code so that it always removes the timer from the idle queue. In my testing of this patch, I put assertions at the points that leon modified in attachment 126533 [details] [diff] [review], and they never fired. The way I see it, the only way for a timer to be in the pipeline more than once, is if it is in the idle queue, and something re-inits the timer ... and perhaps if SetDelay() is called, which this patch doesn't cover. Timer guru's let me know it should be added to SetDelay() too. Anyways, this patch fixes the one shot timer assertion problem we see with composer, even without leon's patch to composer ... though his composer patch should go in too.

Also, I forgot to ask, are we not also concerned about other nodes (blocks, and other inlines) that could have inline styles with display:none?

Comment on attachment 126657 [details] [diff] [review] Patch that removes timer from the mIdleTimers list. about this patch: 1) there are three steps when a timer is used: step 1: create/reuse a timer, and enqueue it into mTimers of timerthread step 2: timerthread(::Run) moved this timer from its mTimers into mIdleTimers of timermanager step 3: timermanager fire each timer in mIdleTimers, then release the timer. (step 1 and step 3 in the same thread, and step 2 in independent timer thread) 2) Current patch only remove a duplicate timer in mIdleTimers during add timer into mTimers or remove timer from mTimers. I am not sure there exist the unique timer in mIdleTimers. e.g. we create or reuse timer1 for blinking caret 5 times (there exist 5 timer1 in mTimers , none in mIdleTimers) since startup. Then these 5 timer1 are moved into mIdleTimers. We call reusing timer 3 times, and this removed 3 timer1 from mIdleTimers. there exist still same timers in mIdleTimers. ..... Current patch can not ensure the unique timer in mTimers: "step 1" is the source of this bug: many same timers are enqueued into mTimers(and then are moved into mIdleTimers in "step 2"). 3) I feel attachment 126533 [details] [diff] [review] is still necessary, because it can ensure there exist only one timer in the mTimers queue of timerthread. I hope the module owners can give some suggestions here.

I think there are many details should be discussed here.

Ignore comment 39 ... wrong bug.

Comment on attachment 125734 [details] [diff] [review] proposed fix sr=kin@netscape.com This should land for mozilla1.5alpha, but we still need to address the timer issues this bug exposed.

kin, can you push authors of timer codes to review our previous related comments and patches?

Comment on attachment 125734 [details] [diff] [review] proposed fix checked in

comments after #35 should be focused.

Comment on attachment 126657 [details] [diff] [review] Patch that removes timer from the mIdleTimers list. requesting brendan to look at this patch written by Kin

Comment on attachment 126533 [details] [diff] [review] check whether there exist same timer in timer array requesting brendan to look at this patch written by Leon Brendan--please review both of these patches and give feedback on which pieces you like / dislike. This bug is currently marked as a blocker for 1.5a.

Comment on attachment 126533 [details] [diff] [review] check whether there exist same timer in timer array This makes nsTimerManager::AddIdleTimer idempotent. Same for TimerThread::AddTimer, but I don't see why that's needed. Isn't the only problem that an idle timer may be re-inited before it has been fired by the idle loop? It should never be the case that a non-idle timer is added twice. So I'd prefer to see the change to TimerThread.cpp just ASSERT that the timer is not already in mTimers. As for making AddIdleTimer idempotent, do we want that? What should happen when an idle timer has no fired yet and it is re-inited? Should it fire? If it shouldn't, shouldn't its position in the mIdleTimers array change to reflect its changed deadline? It might even be necessary, if we want to prefer deadline order, to fire the "old" timer (since its deadline has probably passed without control flow returning to the idle loop) and then requeue the "new" timer (same nsTimerImpl instance, new deadline). On to the next patch, which I think I prefer because it avoids these issues, choosing to auto-cancel idle as well as non-idle timers when they're re-inited. /be

Comment on attachment 126657 [details] [diff] [review] Patch that removes timer from the mIdleTimers list. >Index: xpcom/threads/nsTimerImpl.cpp >=================================================================== >RCS file: /cvsroot/mozilla/xpcom/threads/nsTimerImpl.cpp,v >retrieving revision 1.30 >diff -u -r1.30 nsTimerImpl.cpp >--- xpcom/threads/nsTimerImpl.cpp 8 Jan 2003 23:14:56 -0000 1.30 >+++ xpcom/threads/nsTimerImpl.cpp 27 Jun 2003 22:40:20 -0000 >@@ -211,7 +211,8 @@ > /** > * In case of re-Init, both with and without a preceding Cancel, clear the > * mCanceled flag and assign a new mGeneration. But first, remove any armed >- * timer from the timer thread's list. >+ * timer from the timer thread's list, and the timer manager's idle timer >+ * list. English nit: "..., and from the timer manager's idle timer list." (added "from" for correct parallelism). > * > * If we are racing with the timer thread to remove this timer and we lose, > * the RemoveTimer call made here will fail to find this timer in the timer >@@ -224,6 +225,10 @@ > */ > if (mArmed) > gThread->RemoveTimer(this); >+ >+ if (gManager) >+ gManager->RemoveIdleTimer(this); This should test 'if (mIdle && gManager)' instead. >+nsresult nsTimerManager::RemoveIdleTimer(nsITimer* timer) >+{ >+ if (!timer) >+ return NS_ERROR_FAILURE; This is bogus code, timer should never by null; same comment for AddIdleTimer, which I realize you didn't add. Please remove both if-return statements. Fix these and I'll sr= the fixed patch. /be

I wanted to verify that we don't suffer bad user-visible effects from either kin's or leon's patch -- that it's ok *not* to fire an idle timer that has not fired yet and is being re-inited. /be

So the patch that's been checked in (see comment 45) should fix the user-visible problem, right? (So we shouldn't need anything else for 1.5alpha?)

Right, I think we're ok for mozilla1.5alpha. We'll address the underlying timer problems when the tree opens for mozilla1.5beta.

kin, brade told me to clean up that old patch(attachment 126657 [details] [diff] [review]) if you do not mind :)

Comment on attachment 127519 [details] [diff] [review] clean up kin's patch according to brendan's comments In Cancel, one small change. >@@ -283,6 +288,9 @@ > if (gThread) > gThread->RemoveTimer(this); > >+ if (gManager) >+ gManager->RemoveIdleTimer(this); Test mIdle in the second if condition, just as with the code in InitCommon. Fix that and sr=brendan@mozilla.org /be

Comment on attachment 127599 [details] [diff] [review] last version according to brendan's comments Kin, give this close scrutiny. I'm not removed enough from the code (I should be r='ing, kin sr='ing -- I'm a reluctant peer, ever since fixing MT bugs in event-based timers). /be

I test current patch using rational's quantify today, and find a perf issue below: 1) The target of reiniting and reusing the same timer is to improve the perf of mozilla, but current patch cannot attain it. After applied the patch(attchment 127599), mozilla's composer consumed more cpu than before when typing chars. The reason is: "nsAutoLock ...." in func "nsTimerManager::RemoveIdleTimer" import new time-consuming process. We have reuse the timer of caret, and removed related related enqueuing and dequeuing process of timer, and decrease the times of calling "nsAutoLock ....", if we apply current patch, this will kill our previous improvment in perf. "nsAutoLock ...." is a mutex lock for thread, but it is very time-consuming in many OS(solaris?) 2) The exact source of this bug is that: there are duplicate timers in mIdleTimers of timer manager. For the reason of perf, we should not import mutext lock("nsAutoLock ....") when enqueue duplicate timer of mIdleTimers. 3) Then, how to do with these duplicate timer? (1) replaced same timer of queue with the duplicate one? (2) or removed old one then append these new one (kin's patch?)? (3) or skip the duplicated one(just like the first part of my old patch attchment 125633)? IMHP, any selections of above is OK, because the mIdleTimers.count() is often small and the position of timer should not influence the result of firing timer. Btw, in current cases of reusing timer, replace the content info (e.g. mTimeout) of timer with new value, should not influnce the application, else, we should NOT reuse timer, e.g. reinit caret timer always be ok when we apply any solutions of three selections above.

Leon, I'm curious how many times we actually do remove something from the idle timer list in your testing. I guess what I'm wondering is if you see an improvement if you move the lock into the block that actually removes the timer like this: nsresult nsTimerManager::RemoveIdleTimer(nsITimer* timer) { - nsAutoLock lock(mLock); PRInt32 i = mIdleTimers.IndexOf(timer); if (i >= 0) { + nsAutoLock lock(mLock); mIdleTimers.RemoveElementAt(i); NS_RELEASE(timer); } return NS_OK; } > 2) The exact source of this bug is that: there are duplicate timers in > mIdleTimers of timer manager. Well, the exact cause of the bug is the fact that on Windows, PLEvents and key events are always processed before any pending idle timers on the main thread. So if we have the caret's blink timer sitting in the mIdleTimer queue, and a key event arrives, the editor's EndUpdateViewBatch() method will hide and then show the caret so that it is erased and redrawn at its new position. The code that turns the caret off, calls mBlinkTimer->Cancel(), which fails to remove the blink timer from the idle queue, so when the caret is turned back on, with a call to mBlinkTimer->InitWithFuncCallback(), which also fails to remove the timer from the idle queue, you end up with another instance of the blink timer in the pipe, and since the mTimer's queue is handled on a separate thread, it's possible for this new timer instance to be pushed into the plevent queue and processed before any existing idle timer events have fired, resulting in multiple instances of the caret timer in the idle queue. Just so we're all on the same page, I did some poking around in the timer code to find all the ways a timer can be added to the mTimers queue: 1. nsTimerImpl::InitCommon() - This gets called whenever you call one of the Timer's Init*() methods. The timer is first removed from the mTimers queue and then re-added. It makes sense to me that if you are canceling or re-initializing a timer that you remove it from *all* queues that it is in. That's what the proposed patch does. 2. nsTimerImpl::Fire() - After the timer's callback is fired, a REPEATING_SLACK timer adds itself back on to the mTimer's queue. I noticed that this code doesn't check to see if the timer was cancelled or added to the mTimers queue during the timer callback, it just adds the timer back on the mTimers queue. Possible bug? 3. nsTimerImpl::PostTimerEvent() - Before posting a PLEvent to handle the firing of the timer, it checks to see if the timer is a REPEATING_PRECISE timer. If it is, it will push the timer back on the mTimer queue, and then post the plevent. This means it's still possible to get more than one instance of a timer in the idle queue. 4. TimerThread::TimerDelayChanged() - If SetDelay() is ever called on a timer, and the timer is not currently firing, TimerDelayChanged() is called, which removes the timer from the mTimer queue, and then adds it back to the mTimer queue. As I said in a previous comment in this bug, we should probably be removing the timer from the idle queue in nsTimerImpl::SetDelay() when !mFiring. I managed to trigger my duplicate timer assertion in AddTimerImpl() during a SetDelay() call while resizing one of the app windows, I don't recall whether it was a Browser or Composer window.

kin, I think current improvment is helpful. Can you give "r"?

kin, I tested many times using quantify in solaris 8(this bug not appear in solaris!!). After compare these data , I found that exist no explict change between average test data without that patch and average test data applied this patch. In fact, we can veryfy this easily earlier: this is a windows bug, and AddIdleTimer()/RemoveTimer were never called, and changed to that function should not influnce the perf in solaris. My fault may be I did not finish the whole PURE tests. Before this bug, I made a test: remove "KillTimer()" in function "nsCaret::StopBlinking()"(becuase, we have reuse this caret timer, and should not Cancel() it again and again!!). But i found explict perf regression. I analyzied the data, and found TimerThread::Run consumed more cpu than before this modification(why?), parts of main thread can comprise and test data is almost the same to before.

After this bug, we should go back to the editor module, and improve perf in that module. I filed a bug 212838, this is our orginal focus.

I can trigger this bug (I think) on this forum: http://episteme.arstechnica.com/eve/ubb.x Reply to a thread and you'll get a form. I can trigger it (the cursor disappears) by inserting a smiley (using the functions above the form). I can then mostly get it to return by inserting some bold text or some other item.

(In reply to comment #63) Forgot to add some details about my system. It's WinXP and I'm using FireFox: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.6) Gecko/20040206 Firefox/0.8

I'm also seeing this on Firefox, WinXP Pro SP1 Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7) Gecko/20040602 Firefox/0.8.0+ (TierMann VC++Tools) And on Thunderbird when editing filter fields. Thunderbird version 0.6+ (20040519) Both built from Aviary branch.