586339 - nsFormHistory leaks, should finalize statements + remove observers

Status: RESOLVED FIXED

(Toolkit :: Form Manager, defect)

Description

[Philipp von Weitershausen [:philikon]]

Sync uses nsFormHistory and has observed leaks in xpcshell tests that involve holding a reference to it. Apart from Sync, the only user of nsFormHistory is nsFormAutoComplete which explicitly nulls the reference on xpcom-shutdown. Sync has to do the same for now, but it'd be good if the leaks were fixed in satchel proper.

Comment 1

[Philipp von Weitershausen [:philikon]]

Attachment: first stab

Here's a first stab from last night. Doesn't get rid of the leaks yet, so we're resorting in nulling references in Sync now.

Comment 2

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Comment on attachment 464871 [details] [diff] [review]
first stab

>+        Services.obs.addObserver(this, "idle-daily", false);
>+        Services.obs.addObserver(this, "formhistory-expire-now", false);
>+        Services.obs.addObserver(this, "profile-before-change", false);

Would it help to just make these all weak refs?

>+    dbClose : function () {
>+        this.log("Finalizing statements");
>+      dump("SATCHEL: finalizing statements\n");
>+        for each ([query, statement] in Iterator(this.dbStmts))
>+            statement.finalize();

So the statement is finalized, but we're still holding a ref to it. What if you did
delete this.dbStmts[query]; in the loop (not sure how that works with an Iterator) or
this.dbStmts = []; after?

>+      dump("SATCHEL: closing db\n");
>+        this.dbConnection.close();

What about trying to nullify that reference too?

Depending on what your leaks look like... other things to try nullifying:
this.dbFile & this.prefBranch

Comment 3

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

This was fixed by ebce91e6af7654495e1a358c6f629ed1a8c0d4de and 7debec3b94bd94f8b43587042e1b0b61001091c6.

Comment 4

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

(In reply to Rafael Ávila de Espíndola (:espindola) from comment #3)
> This was fixed by ebce91e6af7654495e1a358c6f629ed1a8c0d4de and
> 7debec3b94bd94f8b43587042e1b0b61001091c6.

Those revisions aren't in mozilla-central, did you paste the wrong thing?

Comment 5

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

they are git revs:

https://github.com/mozilla/mozilla-central/commit/ebce91e6af7654495e1a358c6f629ed1a8c0d4de
https://github.com/mozilla/mozilla-central/commit/7debec3b94bd94f8b43587042e1b0b61001091c6

Comment 6

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

It's really confusing to use git revs in Bugzilla - hg.mozilla.org/mozilla-central is the canonical repository.

http://hg.mozilla.org/mozilla-central/rev/bb4b4b65beed
http://hg.mozilla.org/mozilla-central/rev/de8e1de24f37

Comment 7

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Using git seems more future proof in light of the current hg perf issues :-)