Closed Bug 386665 Opened 15 years ago Closed 15 years ago

Anti-phishing service fails to clean up observers, so some DOMWINDOWs are leaked until shutdown

Categories

(Toolkit :: Safe Browsing, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: dbaron, Unassigned)

References

Details

(Keywords: memory-leak)

Attachments

(4 files)

I'm seeing the count from the DOMWINDOW count printfs in debug builds showing that we're leaking DOM windows when we open and close browser windows.  They all get cleaned up on shutdown, but most (but not all) of the time, I'm seeing the count increase by two for every iteration of (on a pretty clean profile, with an about:blank homepage):

 1. Press Ctrl-N
 2. Click in the window
 3. Press Ctrl-W or close the window with the [X] in the corner

if I have the default --enable-official-branding google/Firefox homepage, then I'm seeing the count increase by 4 for each iteration.
Flags: blocking1.9?
Depends on: 387223
Depends on: 387224
Attached file debugging output
This is debugging output with the patch from bug 387224 (and with bug 387223 fixed).  The vast majority of the unexplained references seem to be from an event listener with a boundSelf_ property, which seems to implicate toolkit/url-classifier (again!).
Attachment #271346 - Attachment is patch: false
Attachment #271346 - Attachment mime type: text/plain → text/plain;charset=UTF-8
I tracked one of the nsXPCWrappedJS in question to see when it was eventually freed -- and it was freed during the cycle collection at shutdown.
This was useful in debugging, since suspecting all wrapped JS, combined with attachment 271387 [details] [diff] [review] from bug 387005, showed (in the ExplainLiveExpectedGarbage output) that collecting just 3 wrapped JS would have solved the leak.  (It listed those wrapped JS as nsIObserver, which they are; I need to look into whether the GC tracing output is listing the wrong interface and the listing of nsIDOMEventListener was bogus, or whether they're different wrappers and there's an additional level of indirection through JS->C++->JS.)
This is the result of breaking in the debugger on the three warnings mentioned above to see what the wrappers were wrapping.  (I wish there were a simpler way to do this.)

The three nsIObserver wrappers were wrapping an object with one property, the anonymous function created in BindToObject in nsUrlClassifierLib.js, which in turn had 5 properties, the last of which was, respectively:
  PROT_PhishingWarden.prototype.onCheckRemotePrefChanged
  PROT_PhishingWarden.prototype.onDataProviderPrefChanged
  PROT_PhishingWarden.prototype.onPhishWardenEnabledPrefChanged
defined in nsSafeBrowsingApplication.js.  It sure looks like nothing is ever responsible from removing these observers, which I think could explain the leak, although I haven't worked the whole thing through (and the safebrowsing code is pretty complicated and it's hard for me to keep track of the lifetimes of all the objects).
Attached patch patchSplinter Review
This seems to help, although cycle collection still seems to take multiple cycles to clean up after things...
Attachment #271393 - Flags: review?(tony)
Component: DOM → Phishing Protection
Flags: blocking1.9?
Product: Core → Firefox
QA Contact: general → phishing.protection
Depends on: 387005
Flags: blocking-firefox3?
This code looks like it's also a problem on the 1.8 branch; need to investigate whether it leaks there.  (If not, I'm curious why not...)
Comment on attachment 271393 [details] [diff] [review]
patch

Is it sufficient to just null out this.prefs_?  None of the other pref observers used by the safe browsing code removes pref observers...
Attachment #271393 - Flags: review?(tony) → review+
Flags: blocking-firefox3? → blocking-firefox3+
No, it's not.  Adding an observer to the pref service means the observer stays around until the pref service goes away.  Any other observers that aren't for the lifetime of the app need the same fixes.
Summary: DOM window leak (++DOMWINDOW) when opening and closing windows → Anti-phishing service fails to clean up observers, so some DOMWINDOWs are leaked until shutdown
(In reply to comment #8)
> No, it's not.  Adding an observer to the pref service means the observer stays
> around until the pref service goes away.  Any other observers that aren't for
> the lifetime of the app need the same fixes.

Ah, ok.  I've seen this before.  It's only a problem for G_Preferences that get instantiated using the default branch.  Other branches produce separate nsPrefService instances:
http://mxr.mozilla.org/seamonkey/source/modules/libpref/src/nsPrefService.cpp#253

I only see G_Preferences with observers in phishing-warden.js and global-store.js (which is global), so that's probably it.  I do, however, see the same problem in thunderbird (mail/components/phishing/phishing-warden.js).
I filed bug 387457 on that inconsistency.
So it seems that this doesn't make us leak on the 1.8 branch, although I honestly have no idea why.  Maybe the JS object munging makes these wrappers such that their parent chain goes up to the JS component's global?

And I checked in the patch above.  Still need to look into whether the same fix is needed for Thunderbird.
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
We'll get this tested (well, for the absence of the leak) in the upcoming work to get regular leak tests that fail when leaks happen, I think, so nothing to do here.
Flags: in-testsuite-
Not really, since this was a leak until partway through shutdown, not all the way through shutdown.
I don't understand the logic behind "in-testsuite- because it's tested by some other tests". If it's tested by other tests it should be marked in-testsuite+. The in-testsuite flag indicates whether the bug is covered by automated tests, not whether the tests are in this bug or another.

(Leaving in-testsuite- given dbaron's comment, but feel free to in-testsuite?)
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.