208446 - Caret stops blinking or disappears

Status: RESOLVED WORKSFORME

(Core :: XPCOM, defect, P3)

Description

[kinmoz]

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.

Comment 1

[kinmoz]

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

Comment 2

[leon.zhang]

ok, I will try.

Comment 3

[leon.zhang]

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

Comment 4

[leon.zhang]

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.

Comment 5

[leon.zhang]

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.
.....

Comment 6

[leon.zhang]

Attachment: go back orginal status

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.

Comment 7

[leon.zhang]

I test it , and current caret is ok.

Comment 8

[leon.zhang]

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.

Comment 9

[leon.zhang]

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.

Comment 10

[Simon Fraser [no longer active]]

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.

Comment 11

[Brendan Eich [:brendan]]

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

Comment 12

[Ere Maijala (slow)]

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.

Comment 13

[Ere Maijala (slow)]

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.

Comment 14

[kinmoz]

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.

Comment 15

[kinmoz]

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

Comment 16

[kinmoz]

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

Comment 17

[leon.zhang]

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).

Comment 18

[leon.zhang]

Attachment: proposed fix

Comment 19

[Kevin McCluskey (gone)]

editorbase+

-> leon.zhang

Comment 20

[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 21

[leon.zhang]

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

no assertion, if this patch is applied.

Comment 22

[leon.zhang]

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 23

[leon.zhang]

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 24

[kinmoz]

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?

Comment 25

[kinmoz]

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?

Comment 26

[Brendan Eich [:brendan]]

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 27

[leon.zhang]

Attachment: an old log file I printed(see line: 21,83,1439,1442,1459,1794)

Comment 28

[leon.zhang]

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.

Comment 29

[leon.zhang]

Attachment: another running log

Comment 30

[leon.zhang]

Attachment: patch can print log info

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

Comment 31

[leon.zhang]

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!!!

Comment 32

[leon.zhang]

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.

Comment 33

[leon.zhang]

  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.

Comment 34

[leon.zhang]

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.

Comment 35

[leon.zhang]

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.

Comment 36

[leon.zhang]

Attachment: Compile error

Comment 37

[leon.zhang]

Attachment: check whether there exist same timer in timer array

Comment 38

[kinmoz]

Attachment: Patch that removes timer from the mIdleTimers list.

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.

Comment 39

[kinmoz]

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 40

[leon.zhang]

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.

Comment 41

[leon.zhang]

I think there are many details should be discussed here.

Comment 42

[kinmoz]

Ignore comment 39 ... wrong bug.

Comment 43

[kinmoz]

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

sr=[e-mail]

This should land for mozilla1.5alpha, but we still need to address the timer
issues this bug exposed.

Comment 44

[leon.zhang]

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

Comment 45

[leon.zhang]

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

checked in

Comment 46

[leon.zhang]

comments after #35 should be focused.

Comment 47

[Kathleen :Brade]

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 48

[Kathleen :Brade]

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 49

[Brendan Eich [:brendan]]

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 50

[Brendan Eich [:brendan]]

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

Comment 51

[Brendan Eich [:brendan]]

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

Comment 52

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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?)

Comment 53

[kinmoz]

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

Comment 54

[leon.zhang]

Attachment: clean up kin's patch according to brendan's comments

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

Comment 55

[Brendan Eich [:brendan]]

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=[e-mail]

/be

Comment 56

[leon.zhang]

Attachment: last version according to brendan's comments

Comment 57

[Brendan Eich [:brendan]]

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

Comment 58

[leon.zhang]

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.

Comment 59

[kinmoz]

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.

Comment 60

[leon.zhang]

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

Comment 61

[leon.zhang]

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.

Comment 62

[leon.zhang]

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.

Comment 63

[Joergen Ramskov]

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.

Comment 64

[Joergen Ramskov]

(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

Comment 65

[Adam]

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.

Comment 66

[Stephen Donner [:stephend] Not actively reading bugmail]

Mass-removing myself from cc; search for 12b9dfe4-ece3-40dc-8d23-60e179f64ac1 or any reasonable part thereof, to mass-delete these notifications (and sorry!)

Comment 67

[Neha Kochar [:neha]]

Really old bug, also not reproducible, so closing.