nsTypeAheadFind leaks by misuse of timers (shutdown leak)

VERIFIED FIXED

Status

--
minor
VERIFIED FIXED
16 years ago
11 years ago

People

(Reporter: dbaron, Assigned: aaronlev)

Tracking

({memory-leak})

Trunk
x86
Linux
memory-leak

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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.)
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.
Keywords: mlk
Summary: nsTypeAheadFind leaks by misuse of timers → nsTypeAheadFind leaks by misuse of timers (shutdown leak)
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.)
(Assignee)

Comment 3

16 years ago
It sets itself up as an oberserver for domwindowopened and domwindowclosed in
::PrefsReset() via nsIWindowWatcher::RegisterNotification()
Blocks: 30088
(Assignee)

Comment 4

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

Seeking r=/sr=
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.
Attachment #102900 - Flags: superreview+

Comment 6

16 years ago
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.
(Assignee)

Comment 7

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

Comment 8

16 years ago
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
Attachment #102900 - Flags: review+
Shouldn't this wait until 1.3alpha?  Also, given comment 6, will it even work
correctly?
(Assignee)

Comment 10

16 years ago
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.
(Assignee)

Comment 11

16 years ago
checked in
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED

Comment 12

16 years ago
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).
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 13

16 years ago
Created attachment 105623 [details] [diff] [review]
1 line patch to fix warning on Linux

Seeking r=/sr=
Attachment #102900 - Attachment is obsolete: true
(Assignee)

Comment 15

16 years ago
Created attachment 105646 [details] [diff] [review]
Oops forgot semicolon
Attachment #105623 - Attachment is obsolete: true
(Assignee)

Comment 16

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

Carrying sr=
r=me
Attachment #105646 - Flags: superreview+
Attachment #105646 - Flags: review+

Comment 17

16 years ago
The follow-up patch was checked in with the bug 176602 patch and the warning
went away.
Status: REOPENED → RESOLVED
Last Resolved: 16 years ago16 years ago
Resolution: --- → FIXED
rs vrfy
Status: RESOLVED → VERIFIED

Updated

16 years ago
Component: Keyboard: Navigation → Keyboard: Find as you Type
Product: Core → SeaMonkey
You need to log in before you can comment on or make changes to this bug.