Password manager no longer requires initialization at startup

VERIFIED FIXED in mozilla32

Status

()

Toolkit
Password Manager
VERIFIED FIXED
5 years ago
4 years ago

People

(Reporter: Dolske, Assigned: Paolo)

Tracking

unspecified
mozilla32
Points:
---
Dependency tree / graph
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: p=1 s=it-32c-31a-30b.1 [qa-])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

5 years ago
Created attachment 731704 [details] [diff] [review]
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.]
(Reporter)

Updated

5 years ago
Depends on: 798844
(Reporter)

Comment 1

5 years ago
Created attachment 804959 [details] [diff] [review]
Patch v.2
Assignee: nobody → dolske
Attachment #731704 - Attachment is obsolete: true
Attachment #804959 - Flags: review?(mark.finkle)
(Reporter)

Updated

5 years ago
Attachment #804959 - Attachment description: noinit.patch → Patch v.2
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.
Attachment #804959 - Flags: review?(mark.finkle) → review+
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.
Depends on: 910521
Permaorange on Linux and Windows, because permaorange everywhere would be far to simple.
(Assignee)

Comment 6

4 years ago
Created attachment 8410195 [details] [diff] [review]
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
Attachment #804959 - Attachment is obsolete: true
Attachment #8410195 - Flags: review?(dolske)
(Reporter)

Comment 7

4 years ago
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).
Attachment #8410195 - Flags: superreview?(jonas)
Attachment #8410195 - Flags: review?(dolske)
Attachment #8410195 - Flags: review+
(Assignee)

Comment 8

4 years ago
(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.
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?
(Assignee)

Comment 10

4 years ago
(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.
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?
(Assignee)

Comment 12

4 years ago
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.
Blocks: 956332
Flags: needinfo?(jonas)
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
I can reproduce when I run all the tests in layout/base/tests/chrome/.
I made some progress in bug 881996 that might unblock this.
Depends on: 881996
No longer depends on: 910521
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.
Attachment #8410195 - Flags: superreview?(jonas) → feedback+
Flags: needinfo?(jonas)
(Assignee)

Comment 17

4 years ago
Created attachment 8420640 [details] [diff] [review]
Patch for checkin

(In reply to :Gavin Sharp (email gavin@gavinsharp.com) 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
Assignee: dolske → paolo.mozmail
Attachment #8410195 - Attachment is obsolete: true
Status: NEW → ASSIGNED
(Assignee)

Updated

4 years ago
Whiteboard: p=1 s=it-32c-31a-30b.1
Hi Juan, can you review this bug if it will require further QA verification.
Flags: needinfo?(jbecerra)
Flags: firefox-backlog+
Whiteboard: p=1 s=it-32c-31a-30b.1 → p=1 s=it-32c-31a-30b.1 [qa?]
https://hg.mozilla.org/mozilla-central/rev/b5f1695e602c
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Qa-'ing considering there's some automated test coverage. We will monitor bugzilla and other feedback for issues on nightly.
Flags: needinfo?(jbecerra)
Whiteboard: p=1 s=it-32c-31a-30b.1 [qa?] → p=1 s=it-32c-31a-30b.1 [qa-]

Updated

4 years ago
Status: RESOLVED → VERIFIED
(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?
(Assignee)

Comment 22

4 years ago
(In reply to :Gavin Sharp (email gavin@gavinsharp.com) 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!
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)"
(Assignee)

Comment 24

4 years ago
(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.
(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 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.
You need to log in before you can comment on or make changes to this bug.