856470 - Password manager no longer requires initialization at startup

Status: VERIFIED FIXED

(Toolkit :: Password Manager, defect)

Description

[Justin Dolske [:Dolske]]

Attachment: Patch v.1

Once bug 839961 lands, password manager will no longer require explicit startup initialization by the application. It has historically required this, because it uses the docloader service to watch for page loads (in order to autofill logins), and so the observer needs to be up and running before loading content pages. Bug 839961 changes how password manager works, so that a frame script automatically hooks up a listener for DOMContentLoaded. Eventually, bug 355063 + bug 771331 will further optimize this by only watching for the addition of a <input type=password> to the page.

As a result, explicit initialization is no longer needed, and we can shave off a little bit of work at startup time.

[Note that password manager will still use a form submit observer to save new logins. But this doesn't need an early init -- you can't submit a form without having triggered a DOMContentLoaded (ignoring the uninteresting edgecase), and post-355063 you can't submit a login form without a password field having been added to it. Both cases will implicitly ensure password manager is initialized and observing before the form is submitted.]

Comment 1

[Justin Dolske [:Dolske]]

Attachment: Patch v.2

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 804959 [details] [diff] [review]
Patch v.2

Looks like desktop, metro and android have all been switched over to LoginManagerContent. This change looks good to me.

Comment 3

[Justin Dolske [:Dolske]]

https://hg.mozilla.org/integration/fx-team/rev/2bdc2ba16e56

Comment 4

[Phil Ringnalda (:philor)]

Pretty sure this'll wind up being fun: backed out in https://hg.mozilla.org/integration/fx-team/rev/e13b2b7c34a4 for making bug 910521 permaorange.

Comment 5

[Phil Ringnalda (:philor)]

Permaorange on Linux and Windows, because permaorange everywhere would be far to simple.

Comment 6

[:Paolo Amadini]

Attachment: New patch

Since this was originally blocked by what was filed as bug 881996, and a workaround was eventually used to make the print preview test work, I think we might as well move the workaround to the test itself, and keep investigating its root cause in bug 881996 and friends.

Tryserver build:

https://tbpl.mozilla.org/?tree=Try&rev=f3d710b15d5c

Comment 7

[Justin Dolske [:Dolske]]

Comment on attachment 8410195 [details] [diff] [review]
New patch

Fine with me, but sicking should weigh in on if this is safe or not. I really don't understand the impact of the assert. If it's really just something unique triggered by print preview weirdness that's fine, but if this is dangerous or can be triggered in other situations that's a problem (ie, the specific test failure could just be a fortuitous red herring, indicating a bigger issue).

Comment 8

[:Paolo Amadini]

(In reply to Justin Dolske [:Dolske] from comment #7)
> (ie, the specific test failure could just be a fortuitous red herring,
> indicating a bigger issue).

It definitely indicates a bigger issue, but if it hasn't been understood and fixed after all the investigation in bug 881996, it's unlikely our team can fix it in a short time. Also, I was unable to reproduce it locally even after building on Windows.

The more conservative alternative would be to keep adding the progress listener synchronously when the browser is window is initialized, exactly like the current workaround does. The fact that it is done in the Login Manager initialization code is what we're trying to avoid. Jonas might know if there is any value in doing this.

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I need more context here. My understanding is that the assert in nsPrincipal::GetAppId is triggered?

Do we know what code is calling GetAppId on the principal? Do we know where that principal got created?

Comment 10

[:Paolo Amadini]

(In reply to Jonas Sicking (:sicking) from comment #9)
> I need more context here. My understanding is that the assert in
> nsPrincipal::GetAppId is triggered?
> 
> Do we know what code is calling GetAppId on the principal? Do we know where
> that principal got created?

The details of our instance of the assertion are in bug 881996. There are other bugs on file for the same assertion, but they might actually refer to different code paths.

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

The stack in bug 881996 comment 0 seems broken.

The stack in bug 881996 comment 4 seems more reliable though. Where does the channel that we reset to come from? It seems like its notification callbacks aren't set to a real docshell. This most likely means that we're not using a real loadgroup when creating that channel?

Comment 12

[:Paolo Amadini]

Jonas, do you think we should keep in the production code the current workaround for the test, or land the current patch that moves the workaround to the test itself?

It's unlikely that we can fix the root issue here, which is in scope of bug 881996 instead.

Comment 13

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

I couldn't reproduce the assertion failure in a linux debug build built today, with the WPL hack removed from attachment 8410195 [details] [diff] [review], and running "mach mochitest-chrome layout/base/tests/chrome/test_printpreview_bug396024.xul".

I pushed that to Try again to see if it still shows up there:
https://tbpl.mozilla.org/?tree=Try&rev=90d0f0027858

Comment 14

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

I can reproduce when I run all the tests in layout/base/tests/chrome/.

Comment 15

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

I made some progress in bug 881996 that might unblock this.

Comment 16

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

Comment on attachment 8410195 [details] [diff] [review]
New patch

Let's not let this issue block this bug any further - landing this workaround is fine for the moment.

Comment 17

[:Paolo Amadini]

Attachment: Patch for checkin

(In reply to :Gavin Sharp (email [e-mail]) from comment #16)
> Let's not let this issue block this bug any further - landing this
> workaround is fine for the moment.

Thank you for helping out with the solution of the Print Preview issue!

I checked in the patch, though I had to take out the delayed initialization on Android because some automated tests show that early initialization is still required by the Java code that accesses the logins database. This does not impact the project to remove synchronous I/O from the Login Manager, because Android will continue to use the mozStorage back-end in any case.

https://hg.mozilla.org/integration/fx-team/rev/b5f1695e602c

Comment 18

[Marco Mucci [:MarcoM]]

Hi Juan, can you review this bug if it will require further QA verification.

Comment 19

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/b5f1695e602c

Comment 20

[juan becerra [:juanb]]

Qa-'ing considering there's some automated test coverage. We will monitor bugzilla and other feedback for issues on nightly.

Comment 21

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

(In reply to :Paolo Amadini from comment #17)
> I checked in the patch, though I had to take out the delayed initialization
> on Android because some automated tests show that early initialization is
> still required by the Java code that accesses the logins database.

This sounds like perhaps a bug that should be filed against Fennec?

Comment 22

[:Paolo Amadini]

(In reply to :Gavin Sharp (email [e-mail]) from comment #21)
> (In reply to :Paolo Amadini from comment #17)
> > I checked in the patch, though I had to take out the delayed initialization
> > on Android because some automated tests show that early initialization is
> > still required by the Java code that accesses the logins database.
> 
> This sounds like perhaps a bug that should be filed against Fennec?

I filed bug 1012677, thanks for the suggestion!

Comment 23

[Mike Kaply [:mkaply]]

Is there some way to force initialization of the password manager?

We have an extension that shows a login dialog before the browser window is up (it's proxy stuff), and we'd like to use the login manager to store credentials.

In Firefox 24 this worked fine, in Firefox 31, we get 

[Exception... "[JavaScript Error: "this._storage is null" {file: "jar:file:///C:/Users/Mike%20Kaply/Desktop/everyware/Browser/omni.ja!/components/nsLoginManager.js" line: 323}]'[JavaScript Error: "this._storage is null" {file: "jar:file:///C:/Users/Mike%20Kaply/Desktop/everyware/Browser/omni.ja!/components/nsLoginManager.js" line: 323}]' when calling method: [nsILoginManager::findLogins]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"

Comment 24

[:Paolo Amadini]

(In reply to Mike Kaply (:mkaply) from comment #23)
> Is there some way to force initialization of the password manager?

You should wait on initializationPromise before making your login dialog visible or enabled:

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/nsILoginManager.idl#18

If you don't wait but use any of the nsILoginManager method, there is still a synchronous fallback, but it may be removed in the future.

Comment 25

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

(In reply to :Paolo Amadini from comment #24)
> If you don't wait but use any of the nsILoginManager method, there is still
> a synchronous fallback, but it may be removed in the future.

Comment 23 seems to suggest that something is going wrong with the synchronous fallback in this case?

Comment 26

[Mike Kaply [:mkaply]]

> Comment 23 seems to suggest that something is going wrong with the synchronous fallback in this case?

It's weirder than that, I realized.

I'm putting my extension in the distribution directory to have it run as early as possible, and it works on Firefox 24 but not Firefox 31. So it's not related to this bug. I'm going to do some debugging to figure out what happened.