385524 - Reduce boilerplate code in passwordmgr

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect)

Description

[Ryan Flint [:rflint] (ping via IRC for reviews)]

Attachment: Patch

Comment 1

[Justin Dolske [:Dolske]]

Comment on attachment 269435 [details] [diff] [review]
Patch

Looks fine to me, but I'll let Gavin handle the official r+ (or r- :-), as I'm not a toolkit peer.

Does XPCOMUtils.jsm have a way to enforce making a component a singleton? We had code for that at one point, but it was removed due to leakage concerns...

Comment 2

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

Comment on attachment 269435 [details] [diff] [review]
Patch

I applied this locally, and didn't notice any leak regressions (though I haven't run it through all of bloaturls).

Comment 3

[Ryan Flint [:rflint] (ping via IRC for reviews)]

mozilla/toolkit/components/passwordmgr/src/nsLoginInfo.js 	1.2 	mozilla/toolkit/components/passwordmgr/src/nsLoginManager.js 	1.3 	mozilla/toolkit/components/passwordmgr/src/storage-Legacy.js 	1.6 	mozilla/toolkit/components/passwordmgr/src/nsLoginManagerPrompter.js 	1.5

I'm going to wait for the perf boxes to chew on this a bit before closing it.

Comment 4

[Ryan Flint [:rflint] (ping via IRC for reviews)]

This nearly doubled RLk, so backed out for now...

mozilla/toolkit/components/passwordmgr/src/nsLoginInfo.js 	1.3 	mozilla/toolkit/components/passwordmgr/src/nsLoginManager.js 	1.4 		mozilla/toolkit/components/passwordmgr/src/storage-Legacy.js 	1.7
mozilla/toolkit/components/passwordmgr/src/nsLoginManagerPrompter.js 	1.6

Comment 5

[Ria Klaassen (not reading all bugmail)]

I see or saw this error:
Error: Cc['@mozilla.org/login-manager;1'] has no properties
Source File: chrome://browser/content/browser.js
Line: 862

Comment 6

[Robert Sayre]

(In reply to comment #5)
> I see or saw this error:
> Error: Cc['@mozilla.org/login-manager;1'] has no properties
> Source File: chrome://browser/content/browser.js
> Line: 862

Does this patch pass the passwordmgr unit tests?

Comment 7

[Nickolay_Ponomarev]

(In reply to comment #1)
> Does XPCOMUtils.jsm have a way to enforce making a component a singleton?

No, why is that needed?

Comment 8

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

(In reply to comment #5)
> I see or saw this error:

I bet this was because we forgot to add XPCOMUtils.jsm to the installer manifests (stupid manifests).

(In reply to comment #7)
> (In reply to comment #1)
> > Does XPCOMUtils.jsm have a way to enforce making a component a singleton?
> 
> No, why is that needed?

It's needed in some cases (e.g. for FUEL) because we want to ensure a singleton even if users call createInstance. In the FUEL case, the "JavaScript global property" code always uses createInstance, yet we want there to be only one instance of FUEL alive (arguably we should provide a way to tell the global property code whether the object is a service or not, but that's another bug). I'm not sure why it would be needed in this case, perhaps someone is createInstance()ing the password manager when they shouldn't be? That might explain the leak increase.

Comment 9

[Nickolay_Ponomarev]

> perhaps someone is createInstance()ing the password manager when they 
> shouldn't be?

I believe that has always been considered a bug in the caller. 

Comment 10

[Ryan Flint [:rflint] (ping via IRC for reviews)]

(In reply to comment #6)
> Does this patch pass the passwordmgr unit tests?
Yes they passed in my build and on the tinderboxen. Gavin's indeed correct that we aren't packaging XPCOMUtils.jsm (bug 385611), which I'm fairly certain caused that error.

Comment 11

[Justin Dolske [:Dolske]]

(In reply to comment #9)
> > perhaps someone is createInstance()ing the password manager when they 
> > shouldn't be?
> 
> I believe that has always been considered a bug in the caller. 

This bug shouldn't exist in the Mozilla source tree, but I was asking because it's the kind of thing a novice extension author might do. Anyway, it's not a critical issue for this bug (as evidenced by that code having already been removed :).

Comment 12

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

The reason I asked about createInstance() was that the packaging problem shouldn't have had any effect on the leaking, since the leak box doesn't test packaged builds, as far as I know. It sounds like the leak is still unresolved.

Comment 13

[Nickolay_Ponomarev]

Attachment: leaking test component

This is somewhat minimized version of nsLoginManager.js, which causes a similar leak when instantiated.

The key is this:
  this.a = Components.classes["@mozilla.org/preferences-service;1"]
.getService(Components.interfaces.nsIPrefService).getBranch("test.");
  this.__proto__.obj = {};
  this.__proto__.obj.backlink = this;
This causes an nsPrefBranch leak, presumably because the |this| js object is itself leaked. (It's similar to nsLoginManager, which creates a few such 'cycles' via _observer, _domEventListener, _webProgressListener).

Presumably, XPCOMUtils plays a part in this, because the leak we saw on tinderbox appeared after the login manager was converted to use it. I don't have time to minimize this test component further.

Hopefully it's more useful than confusing. I'd appreciate if someone could confirm this indeed leaks, and that it leaks due to the __proto__ lines - the leak stats are not particularly stable.

Comment 14

[Robert Sayre]

(In reply to comment #13)
> 
> Presumably, XPCOMUtils plays a part in this, because the leak we saw on
> tinderbox appeared after the login manager was converted to use it.

Turns out that nsLoginManager leaks without XPCOMUtils...

Comment 15

[Nickolay_Ponomarev]

Likely (at the very least there was a patch somewhere to convert the addObserver calls to use a weak reference), but in my testing the nsPrefBranch leak did not occur without XPCOMUtils.

Comment 16

[Ryan Flint [:rflint] (ping via IRC for reviews)]

With the checkin for bug 180380 this no longer has any negative impact on any of the perf/leak tests. Go Sayre!

mozilla/toolkit/components/passwordmgr/src/nsLoginInfo.js 	1.4 	mozilla/toolkit/components/passwordmgr/src/nsLoginManager.js 	1.9 	mozilla/toolkit/components/passwordmgr/src/nsLoginManagerPrompter.js 	1.7 	mozilla/toolkit/components/passwordmgr/src/storage-Legacy.js 	1.9