1527828 - Remove insecure password field detection code for the address bar

Status: RESOLVED FIXED

(Toolkit :: Password Manager, enhancement, P2)

Description

[Matthew N. [:MattN]]

_detectInsecureFormLikes[1] runs on every pageshow and from the DOMFormHasPassword event and shows up in this profile from smaug: http://bit.ly/2DI53jz

It's also not going to work properly with Fission IIUC since it traverses window.frames.

IMO this logic which is mostly independent of password manager should be rewritten in C++ and work more like the mixed content address bar indicator. That should provide page load speed improvements.

Test pages:

• http://http-login.badssl.com/
• http://http-password.badssl.com/
• http://http-dynamic-login.badssl.com/

You should see a broken lock icon in the address bar AND the identity panel should say "Logins entered on this page could be compromised" inside.
I don't have a test-page with subframes handy but you can probably embed those test pages in an <iframe> on an http: server.

[1] https://searchfox.org/mozilla-central/rev/01b4b3830ea3cae2e9e431019afa6391b471c6da/toolkit/components/passwordmgr/LoginManagerContent.jsm#483-505

Comment 1

[Matthew N. [:MattN]]

On a related note, the insecure login field console warnings from InsecurePasswordUtils.jsm could also be consolidated with the address bar indicator logic and we could avoid loading that JSM on page load too. There is also telemetry being accumulated on every page load in InsecurePasswordUtils which could have a perf impact.

Comment 2

[Matthew N. [:MattN]]

In bug 1555317 comment 27 we are discussing whether we can now remove this code altogether… doing so could help pageload performance and then we wouldn't have to fix it for Fission.

Comment 3

[Matthew N. [:MattN]]

We got the go-ahead to remove this code now (bug 1555317 comment 28) so we don't have to fix it for Fission and we can reduce the work we do on page loads and tab switches.

Comment 4

[Johann Hofmann [:johannh]]

So is this a dupe of bug 1567827 now? :)

Comment 5

[Matthew N. [:MattN]]

Maybe, this bug has the context for performance and focused on the passwordmgr/ content process portion whereas bug 1567827 is more about the doorhanger parent process portion. They can be done in one bug or separate depending on the dev. who takes it.

Comment 6

[Matthew N. [:MattN]]

Deleting this code allows us to delete 3 failing tests in Fission so moving to M4.

Comment 7

[Matthew N. [:MattN]]

Attachment: Bug 1527828 - Remove insecure password field detection code for the address bar. r=sfoster

Comment 8

[Matthew N. [:MattN]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a2a39c851dd1cf2f306d106127ffb290919d72f8

Comment 9

[Sam Foster [:sfoster] (he/him)]

Attachment: A secure form with an iframe'd insecure form

Test page that frames an insecure form at http://http-login.badssl.com/

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d80e1918c41d
Remove insecure password field detection code for the address bar. r=sfoster

Comment 11

[Natalia Csoregi [:nataliaCs]]

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