Closed Bug 385524 Opened 16 years ago Closed 16 years ago

Reduce boilerplate code in passwordmgr

Categories

(Toolkit :: Password Manager, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9alpha7

People

(Reporter: rflint, Assigned: rflint)

References

Details

Attachments

(2 files)

Attached patch PatchSplinter Review
No description provided.
Attachment #269435 - Flags: review?(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...
Attachment #269435 - Flags: review?(gavin.sharp)
Attachment #269435 - Flags: review?(dolske)
Attachment #269435 - Flags: review+
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).
Attachment #269435 - Flags: review?(gavin.sharp) → review+
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.
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
Depends on: 380969
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
(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?
(In reply to comment #1)
> Does XPCOMUtils.jsm have a way to enforce making a component a singleton?

No, why is that needed?
(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.
> 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. 
(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.

(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 :).
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.
Attached file 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.
(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...
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.
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 
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3 M7
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.