Closed Bug 174145 Opened 20 years ago Closed 20 years ago

nsTypeAheadFind leaks by misuse of timers (shutdown leak)

Categories

(SeaMonkey :: Find In Page, defect)

x86
Linux
defect
Not set
minor

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: dbaron, Assigned: aaronlev)

References

Details

(Keywords: memory-leak)

Attachments

(1 file, 2 obsolete files)

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.)
It sets itself up as an oberserver for domwindowopened and domwindowclosed in
::PrefsReset() via nsIWindowWatcher::RegisterNotification()
Blocks: isearch
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+
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.
Shouldn't we be fixing the timer code so that SetDelay() uncancels the timer?
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?
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.
checked in
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
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 → ---
Seeking r=/sr=
Attachment #102900 - Attachment is obsolete: true
Comment on attachment 105623 [details] [diff] [review]
1 line patch to fix warning on Linux

sr=dbaron
Attachment #105623 - Flags: superreview+
Attachment #105623 - Attachment is obsolete: true
Comment on attachment 105646 [details] [diff] [review]
Oops forgot semicolon

Carrying sr=
r=me
Attachment #105646 - Flags: superreview+
Attachment #105646 - Flags: review+
The follow-up patch was checked in with the bug 176602 patch and the warning
went away.
Status: REOPENED → RESOLVED
Closed: 20 years ago20 years ago
Resolution: --- → FIXED
rs vrfy
Status: RESOLVED → VERIFIED
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.