394603 - Login Manager should be started from a category.

Status: RESOLVED WONTFIX

(Toolkit :: Password Manager, defect, P5)

Description

[Justin Dolske [:Dolske]]

Comment 1

[Justin Dolske [:Dolske]]

Spun off from bug 380931:

* Login Manager should be started from a category in nsHTMLFormElement.cpp
(like the old password manager), instead of being launched at startup from
browser.js.

Comment 2

[Justin Dolske [:Dolske]]

Attachment: Patch, work in progress

* Changed satchel to not cache the pwmgr on startup... browser.xml's attachFormFill() causes satchel to be created during startup (bug?), which in turn was causing pwmgr to be created during startup.

* Not quite working: when nsHTMLFormElement triggers the creation of Login Manager, the page being loaded doesn't get processed by Login Manager. Need to is it's WebProgressListener should be getting triggered, or if we have to handle this "first init" as a special case.

* If we set service=true for the XPCOMUtils category stuff, this doesn't work. nsHTMLFormElement is calling NS_CreateServiceFromCategory(), which works fine until it ends up calling |do_getService("service,@mozilla.org/login-manager;1")| which fails. The "service," prepended by XPCOMUtils doesn't seem to be understood here, and I'm not really sure what it's supposed to be doing... Other code works (?) with this, usually with the "app-startup" category.

Comment 3

[Robert Sayre]

(In reply to comment #1)
> Spun off from bug 380931:
> 
> * Login Manager should be started from a category in nsHTMLFormElement.cpp
> (like the old password manager), instead of being launched at startup from
> browser.js.
> 

Why?

Comment 4

[Justin Dolske [:Dolske]]

(In reply to comment #3)

> Why?

Because there's no reason to create the service until it's needed, and the current code is likely causing a minor performance hit.

Comment 5

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

So that means that we should probably force Tp to initialize the password manager before it starts its tests, so that we don't charge the first hapless page with a password input for the PM's initialization cost, IMO.  I trust that someone involved in this change will make sure that appropriate changes are made to our test setup. :)

Comment 6

[Justin Dolske [:Dolske]]

Hmm, so, I wonder if this is really worth fixing.

1) Starting from a category could make startup (Ts) faster... I don't know what the cost is of creating the Login Manager service, but if it's so small as to be noise there's not much point in optimizing this. Should be easy enough to measure the cost with DTrace.

2) Delaying creation until the first password field could be a perf win. That seems a little silly from a real-world PoV, since so many pages have password fields. Saving a few milliseconds on the first 10 pages you visit doesn't seem interesting.

[I agree that Login Manager should be running during Tp measurement, since it would most likely be running in a real user's browsing session anyway.]

Comment 7

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

(In reply to comment #6)
> 2) Delaying creation until the first password field could be a perf win. That
> seems a little silly from a real-world PoV, since so many pages have password
> fields. Saving a few milliseconds on the first 10 pages you visit doesn't seem
> interesting.

It's also that we (really the user) would likely pay the cost when we're waiting for network data anyway, since it would be right after the creation of the element (by which point we've likely kicked off requests for header images, etc.). I think we should do the right thing here, and Gavin has argued quite passionately that this is The Right Thing to do, so I expect him to be persuasive as well.

Comment 8

[Justin Dolske [:Dolske]]

I took a look at the DTrace of startup sayrer did; see http://people.mozilla.com/~sayrer/2007/09/30/results.txt. [I hope I didn't misinterpret what was being measured.]

The sum of the left column was 2199 ms [throwing out the first value, 2040ms, since I assume that was waiting for DTrace to kick in or something?].

I then extracted just the LoginManager section...

79602        browser.xml                  |  -> getService
1545584      nsLoginManager.js            |   -> import
...
109898       browser.xml                  |  <- getService

The sum of the left column for this section was 4.6ms, or 0.21% of the total time.

It's also interesting that 3.2ms of that 4.6ms (70%) was spent before even calling nsIFactory::createInstance. [And of that 3.2ms, about half was spent between calling getService() and the first script execution.]

Comment 9

[Justin Dolske [:Dolske]]

I think this is all-risk and no-gain at this point, so futureing.

Comment 11

[Matthew N. [:MattN]]

I'm removing the category and observer notifications in bug 1474143 where the content process aspect of password manager will be loaded lazily using a DOM event instead since that is more compatible with Fission.