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

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect)

Description

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

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.

Comment 1

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

Attachment: 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!).

Comment 2

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

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.

Comment 3

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

Attachment: useful debugging patch: suspect all wrapped JS

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

Comment 4

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

Attachment: debugging output pinpointing the problem

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

Comment 5

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

Attachment: patch

This seems to help, although cycle collection still seems to take multiple cycles to clean up after things...

Comment 6

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

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 7

[Tony Chang (Google)]

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

Comment 8

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

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.

Comment 9

[Tony Chang (Google)]

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

Comment 10

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

I filed bug 387457 on that inconsistency.

Comment 11

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

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.

Comment 12

[Jeff Walden [:Waldo]]

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.

Comment 13

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

Not really, since this was a leak until partway through shutdown, not all the way through shutdown.

Comment 14

[:Gavin Sharp [email: [e-mail]]]

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