174145 - nsTypeAheadFind leaks by misuse of timers (shutdown leak)

Status: VERIFIED FIXED

(SeaMonkey :: Find In Page, defect)

Description

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

nsTypeAheadFind leaks by misuse of timers in two ways:
 * it reinitializes the same timer in its StartTimeout method, which adds an
additional reference to the nsTypeAheadFind object each time.  (Timers should
really assert when you do this!)  I *think* you can do what this does by usingw
|SetDelay|, but I'm not sure.
 * It never breaks the ownership cycle between itself and its timer.  (The easy
way to do this would be to null out the timer pointer in the Notify method, but
that would mean a lot more churn of timer creation.)

Comment 1

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

Note that this is only a one-time leak (the same objects, although one of them
gets many references leaked), so I've marked it as minor.

Comment 2

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

Since nsTypeAheadFind already implements nsIObserver, it would be very easy to
null out the timer by observing XPCOM shutdown (NS_XPCOM_SHUTDOWN_OBSERVER_ID).
 (Speaking of observing, I don't understand how the nsTypeAheadFind gets
"domwindowopened" and "domwindowclosed" notifications.  From looking at the
code, it seems like magic.)

Comment 3

[Aaron Leventhal]

It sets itself up as an oberserver for domwindowopened and domwindowclosed in
::PrefsReset() via nsIWindowWatcher::RegisterNotification()

Comment 4

[Aaron Leventhal]

Attachment: Adds shutdown method via nsIObserver/NS_XPCOM_SHUTDOWN_OBSERVER_ID, use nsITmer::SetDelay() instead of Initializing new timers

Seeking r=/sr=

Comment 5

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

Comment on attachment 102900 [details] [diff] [review]
Adds shutdown method via nsIObserver/NS_XPCOM_SHUTDOWN_OBSERVER_ID, use nsITmer::SetDelay() instead of Initializing new timers

>-      observerService->AddObserver(typeAheadFind, "nsWebBrowserFind_FindAgain", PR_TRUE);
>+      observerService->AddObserver(typeAheadFind, "nsWebBrowserFind_FindAgain", 
>+                                   PR_TRUE);
>+      observerService->AddObserver(typeAheadFind, NS_XPCOM_SHUTDOWN_OBSERVER_ID, 
>+                                   PR_FALSE);

I don't see any reason for the second to be PR_FALSE.  Weak references
introduce extra cost, and if it already has one reference, two won't do any
harm.  Given that change to PR_TRUE, sr=dbaron.

Comment 6

[Kyle Yuan]

This patch doesn't fix the |mTimer->Cancel()| problem. I checked with 
nsITimer's implementation 
http://lxr.mozilla.org/seamonkey/source/xpcom/threads/nsTimerImpl.cpp#266
once we called Cancel(), mCanceled was set to PR_TRUE; and no way to set it 
back to PR_FALSE except the cstr. This flag controls whether the timeout event 
can be fired, see
http://lxr.mozilla.org/seamonkey/source/xpcom/threads/nsTimerImpl.cpp#332

So either we should use |mTimer = nsnull| after |mTimer->Cancel()|, or use 
mTimer->SetDelay(aVeryLongDelay) instead.

Comment 7

[Aaron Leventhal]

Shouldn't we be fixing the timer code so that SetDelay() uncancels the timer?

Comment 8

[Kyle Yuan]

Comment on attachment 102900 [details] [diff] [review]
Adds shutdown method via nsIObserver/NS_XPCOM_SHUTDOWN_OBSERVER_ID, use nsITmer::SetDelay() instead of Initializing new timers

r=kyle

Comment 9

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

Shouldn't this wait until 1.3alpha?  Also, given comment 6, will it even work
correctly?

Comment 10

[Aaron Leventhal]

David, this can wait until 1.3 alpha.

Comment #6 relates to another timer problem dealing with IME in the type ahead
find code. I had asked Kyle to see if this fixed his problem. The original bug
is fixed with this patch, however.

Comment 11

[Aaron Leventhal]

checked in

Comment 12

[Aleksey Nogin]

If I am not mistaken, this checkin have added a warning to brad TBox:

+extensions/typeaheadfind/src/nsTypeAheadFind.cpp:347
+ `PRBool isOpening' might be used uninitialized in this function

IMHO to solve this in the branch

+  else if (!nsCRT::strcmp(aTopic, NS_XPCOM_SHUTDOWN_OBSERVER_ID)) {
+    Shutdown();
+  }

it needs to set isOpening to PR_FALSE (or return NS_OK right away).

Comment 13

[Aaron Leventhal]

Attachment: 1 line patch to fix warning on Linux

Seeking r=/sr=

Comment 14

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

Comment on attachment 105623 [details] [diff] [review]
1 line patch to fix warning on Linux

sr=dbaron

Comment 15

[Aaron Leventhal]

Attachment: Oops forgot semicolon

Comment 16

[Aaron Leventhal]

Comment on attachment 105646 [details] [diff] [review]
Oops forgot semicolon

Carrying sr=
r=me

Comment 17

[Aleksey Nogin]

The follow-up patch was checked in with the bug 176602 patch and the warning
went away.

Comment 18

[sairuh (rarely reading bugmail)]

rs vrfy